kicad-developers team mailing list archive

[Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>]

I think I've got something that works now.  The _D patch attached fixes
the UTF8 issues in kicad2step provided the patch to the wxWidgets
header (also attached) is applied.  If the wx patch is not applied then
the kicad2step code is technically fixed in kicad but will not work due
to wx - however, the build is not broken on any platform/toolchain.

The wx patch (or something similar to it) will hopefully be applied by
the wxWidgets team but in the meantime we can patch the wx/app.h
header on our Windows builds. Patching of this header file does not
require any recompilation of wx components and the patch should be
entirely transparent to all existing projects which use
wxIMPLEMENT_APP_CONSOLE. The wx patch has been tested on
both MinGW32 and MinGW64 and has no effects on any other
platform/toolchain.

- Cirilo

On Thu, Mar 9, 2017 at 9:50 AM, Cirilo Bernardo
<cirilo.bernardo@xxxxxxxxx> wrote:
> On Thu, Mar 9, 2017 at 7:53 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>> On 3/8/2017 3:18 PM, Cirilo Bernardo wrote:
>>> On Thu, Mar 9, 2017 at 3:18 AM, Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>>>> Cirilo,
>>>>
>>>> For what it's worth, it almost seems like it would have been less work
>>>> to just export the step file directly from pcbnew rather than the effort
>>>> you made to get all of this to work.  I do have a few comments below.
>>>>
>>>> On 3/8/2017 8:33 AM, Cirilo Bernardo wrote:
>>>>> I'm confident that I can now fix all remaining non-ASCII
>>>>> filename issues in KiCad *and* use the correct initialization
>>>>> macro in kicad2step (wxIMPLEMENT_APP_CONSOLE).
>>>>>
>>>>> This will require some coordination in order to avoid
>>>>> breaking the automatic build for Windows.
>>>>>
>>>>> Step A: Implement the patch attached to fix the wxWidgets
>>>>> header 'wx/app.h'.
>>>>
>>>> Did you cherry pick this patch from upstream wxWidgets?  If not, did you
>>>> test it to ensure that it did not break anything else?  I don't know if
>>>> it makes sense or not to create a pull request with the msys2 project to
>>>> patch wxWidgets.  I don't want to be responsible for breaking existing
>>>> code but I would rather not have to maintain my own custom build of
>>>> wxWidgets on msys2 either.  Also, if this isn't cherry picked from
>>>> wxWidgets, you probably should file a bug report with this patch as a
>>>> potential fix.
>>>>
>>>
>>> The patch is not from wxWidgets but I've posted to the appropriate
>>> open wxWidgets ticket. There is a side effect that the patch will
>>> break the build of any wxWidgets console app which uses
>>> wxIMPLEMENT_APP_CONSOLE *and* does not already pass the
>>> -municode flag to the GCC linker.
>>>
>>> I've just got some feedback on wxWidgets that the patch won't
>>> work because MinGW32 doesn't provide wmain() and the devs
>>> aren't keen on making people use the -municode flag on MinGW64.
>>
>> I kind of see their point.  I figured it was a long shot but it was
>> worth a try.
>>
>>>
>>>
>>>>>
>>>>> Step B: Push the *_C patch to fix UTF8 problems in kicad2step
>>>>> and ensure that kicad2step and idf2vrml build with the already
>>>>> patched wx header. Ignore previous related patches; this _C
>>>>> patch has them all.
>>>>
>>>> Will this patch break kicad2step and idf2vrml without the patch above
>>>> applied to wxWidgets?
>>>>
>>>
>>> Yes; without the wxWidgets patch passing -municode will break
>>> kicad2step and idf2vrml ('wmain not defined'). If we still support
>>> Win32 then these patches aren't helpful at all at this point.
>>>
>>> There is one more scheme I can investigate which might work on
>>> MinGW32 as well. Sorry about all the noise. I hope I can find a
>>> solution which fixes the problem in wxWidgets so we're not left
>>> with ugly workarounds.
>>
>> Please investigate.  Your current solutions are not the most desirable
>> option.  If it's a last resort, then maybe we can live with it but it
>> would be nice not have a custom wxWidgets on mingw if we don't have to.
>>
>> I wonder if this issue applies to all externally launched applications
>> (python scripts, xsltpoc, etc.) on mingw not just kicad2step and idf2vrml.
>>
>
> I agree; we don't want a custom wx on MinGW32/64; having an OCE hack
> is bad enough.
>
> I'm looking into this and currently setting up MinGW32 as well so I can
> test my solutions more before proposing another patch. I'm still hoping a
> simple patch to the wx/app.h header is all that is needed. I've implemented
> a workaround in idf2vrml and kicad2step as a proof of concept, but it's
> an ugly and cumbersome workaround.
>
> The issue affects all external wxConsoleApps.  I'm also aware of a few
> of my own tools like dxf2idf which do not use wxConsoleApp but are
> nonetheless broken in the same way. If there is no great urgency to
> fix the problem I may take a week or two to review all our external apps
> and fix them up.
>
> - Cirilo
>
>>>
>>> - Cirilo
>>>
>>>>>
>>>>> Once that is done, all the major filename problems in MinGW
>>>>> will be fixed and I will continue to fix up the IDF tools which
>>>>> seem to be rarely used but do contain UTF8 problems.
>>>>>
>>>>> - Cirilo
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>
>>>>
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>> More help   : https://help.launchpad.net/ListHelp

diff --git a/include/wx/app.h b/include/wx/app.h
index 9a73469570..89c1762dc7 100644
--- a/include/wx/app.h
+++ b/include/wx/app.h
@@ -798,6 +798,19 @@ public:
                                                                               \
             return wxEntry(argc, argv);                                       \
         }
+#elif wxUSE_UNICODE && ( defined(__MINGW32__) || defined(__MINGW64__) )
+    #define wxIMPLEMENT_WXWIN_MAIN_CONSOLE                                    \
+        int main(int argc, char **argv)                                       \
+        {                                                                     \
+            wxDISABLE_DEBUG_SUPPORT();                                        \
+                                                                              \
+            LPWSTR cmdline = ::GetCommandLineW();                             \
+            int argcw;                                                        \
+            LPWSTR* argvw = ::CommandLineToArgvW( cmdline, &argcw );          \
+            int result = wxEntry( argcw, argvw );                             \
+            ::LocalFree( argvw );                                             \
+            return result;                                                    \
+        }
 #else // Use standard main()
     #define wxIMPLEMENT_WXWIN_MAIN_CONSOLE                                    \
         int main(int argc, char **argv)                                       \

From baba8555d49c64c6f175025a6e3d47b3bb5ced83 Mon Sep 17 00:00:00 2001
From: Cirilo Bernardo <cirilo.bernardo@xxxxxxxxx>
Date: Thu, 9 Mar 2017 13:40:41 +1100
Subject: [PATCH] Fix UTF8 filename issues in kicad2step

---
 utils/kicad2step/CMakeLists.txt      | 13 ++++++++++---
 utils/kicad2step/pcb/kicadmodule.cpp |  3 ++-
 utils/kicad2step/pcb/oce_utils.cpp   | 10 +++++-----
 3 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/utils/kicad2step/CMakeLists.txt b/utils/kicad2step/CMakeLists.txt
index bdfff3390..a60153f42 100644
--- a/utils/kicad2step/CMakeLists.txt
+++ b/utils/kicad2step/CMakeLists.txt
@@ -1,13 +1,14 @@
 include_directories( BEFORE
-        pcb
-        ${CMAKE_CURRENT_SOURCE_DIR}
+    pcb
+    ${CMAKE_CURRENT_SOURCE_DIR}
+    ${CMAKE_SOURCE_DIR}/include
 )
 
 include_directories( SYSTEM 
     ${OCE_INCLUDE_DIRS}
 )
 
-add_executable( kicad2step
+set( K2S_FILES
     kicad2step.cpp
     pcb/3d_resolver.cpp
     pcb/base.cpp
@@ -21,6 +22,12 @@ add_executable( kicad2step
     sexpr/sexpr_parser.cpp
 )
 
+if( MINGW )
+    list( APPEND K2S_FILES ${CMAKE_SOURCE_DIR}/common/streamwrapper.cpp )
+endif( MINGW )
+
+add_executable( kicad2step ${K2S_FILES} )
+
 target_link_libraries( kicad2step ${wxWidgets_LIBRARIES} ${LIBS_OCE} )
 
 if( APPLE )
diff --git a/utils/kicad2step/pcb/kicadmodule.cpp b/utils/kicad2step/pcb/kicadmodule.cpp
index 532b02c9c..03ffff415 100644
--- a/utils/kicad2step/pcb/kicadmodule.cpp
+++ b/utils/kicad2step/pcb/kicadmodule.cpp
@@ -360,7 +360,8 @@ bool KICADMODULE::ComposePCB( class PCBMODEL* aPCB, S3D_RESOLVER* resolver,
 
     for( auto i : m_models )
     {
-        std::string fname( resolver->ResolvePath( i->m_modelname.c_str() ).ToUTF8() );
+        std::string fname( resolver->ResolvePath(
+            wxString::FromUTF8Unchecked( i->m_modelname.c_str() ) ).ToUTF8() );
 
         if( aPCB->AddComponent( fname, m_refdes, LAYER_BOTTOM == m_side ? true : false,
             newpos, m_rotation, i->m_offset, i->m_rotation ) )
diff --git a/utils/kicad2step/pcb/oce_utils.cpp b/utils/kicad2step/pcb/oce_utils.cpp
index 8fa45cb04..cce6d1fd2 100644
--- a/utils/kicad2step/pcb/oce_utils.cpp
+++ b/utils/kicad2step/pcb/oce_utils.cpp
@@ -31,6 +31,7 @@
 
 #include "oce_utils.h"
 #include "kicadpad.h"
+#include "streamwrapper.h"
 
 #include <IGESCAFControl_Reader.hxx>
 #include <IGESCAFControl_Writer.hxx>
@@ -153,7 +154,7 @@ enum FormatType
 
 FormatType fileType( const char* aFileName )
 {
-    wxFileName lfile( aFileName );
+    wxFileName lfile( wxString::FromUTF8Unchecked( aFileName ) );
 
     if( !lfile.FileExists() )
     {
@@ -172,16 +173,15 @@ FormatType fileType( const char* aFileName )
     else if( ext == "emn" || ext == "EMN" )
         return FMT_EMN;     // PCB assembly
 
-    std::ifstream ifile;
-    ifile.open( aFileName );
+    OPEN_ISTREAM( ifile, aFileName );
 
-    if( !ifile.is_open() )
+    if( ifile.fail() )
         return FMT_NONE;
 
     char iline[82];
     memset( iline, 0, 82 );
     ifile.getline( iline, 82 );
-    ifile.close();
+    CLOSE_STREAM( ifile );
     iline[81] = 0;  // ensure NULL termination when string is too long
 
     // check for STEP in Part 21 format
-- 
2.11.0