Patchwork D7444: setup: conditionalize access to `sys.dllhandle` when building extensions

login
register
mail settings
Submitter phabricator
Date Nov. 16, 2019, 5:29 p.m.
Message ID <differential-rev-PHID-DREV-47mwfug5zmwt5cjzln6h-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43303/
State Superseded
Headers show

Comments

phabricator - Nov. 16, 2019, 5:29 p.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This code is only run on Windows, and was crashing PyOxidizer when running in
  `setup-py-install` mode.  Now an oxidized binary can be built by simply pointing
  to setup.py.
  
  Something is slightly different now that it's not being built from a virtualenv.
  Previously, `hg version` could print to the screen, but now it aborts saying
  "Incorrect function".  But I can see the output if redirected to a file, and
  it's not complaining about missing C extensions, so I think those are loading
  now (unlike from the virtualenv).  The interesting this about this incorrect
  function output is that it failed when initially built.  I then went back and
  did a `make clean` and `make local` with py3 and then py2 to ensure I didn't
  break the existing code.  At that point I ran the oxidized executable again and
  it was able to print to the screen normally!  So I ran `pyoxidizer build` again,
  it only output the following, and then running the executable failed to output
  again:
  
    (pyO2_venv) C:\Users\Matt\hg3\hg_pyO2>pyoxidizer build
        Finished dev [unoptimized + debuginfo] target(s) in 0.12s
    packaging application into C:/Users/Matt/hg3/hg_pyO2\build\apps\hg_pyO2\x86_64-pc-windows-msvc\debug
    purging C:/Users/Matt/hg3/hg_pyO2\build\apps\hg_pyO2\x86_64-pc-windows-msvc\debug
    copying C:/Users/Matt/hg3/hg_pyO2\build\target\x86_64-pc-windows-msvc\debug\hg_pyO2.exe to
        C:/Users/Matt/hg3/hg_pyO2\build\apps\hg_pyO2\x86_64-pc-windows-msvc\debug\hg_pyO2.exe
    resolving packaging state...
    writing license for [...]
    hg_pyO2 packaged into C:/Users/Matt/hg3/hg_pyO2\build\apps\hg_pyO2\x86_64-pc-windows-msvc\debug
    executable path: C:/Users/Matt/hg3/hg_pyO2\build\apps\hg_pyO2\x86_64-pc-windows-msvc\debug\hg_pyO2.exe

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7444

AFFECTED FILES
  setup.py

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 16, 2019, 7:19 p.m.
This revision is now accepted and ready to land.
indygreg added a comment.
indygreg accepted this revision.


  This workaround is fine for PyOxidizer. But `pythonlib` and `hgpythonlib.h` and `exewrapper.c` aren't needed for PyOxidizer. So there is further room to make large parts of this code conditional on running in PyOxidizer. The path I was going in my patch was to have `setup.py` look for an environment variable that identified the current build environment as PyOxidizer and tweak behavior appropriately.
  
  I do like the approach of making this patch agnostic of PyOxidizer, as other environments could also lack a `sys.dllhandle`. And arguably the proper way to disable the building of the exe wrapper is to add an argument to our `setup.py` to control it. PyOxidizer could then pass in this argument.
  
  As for the output issues, I'm not sure what's going on. I added support for `Py_LegacyWindowsStdioFlag` to PyOxidizer a few hours ago. I'd start there. Feel free to make noise on PyOxidizer's issue tracker!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7444/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7444

To: mharbison72, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Nov. 17, 2019, 2:01 a.m.
mharbison72 added a comment.


  In D7444#109357 <https://phab.mercurial-scm.org/D7444#109357>, @indygreg wrote:
  
  > As for the output issues, I'm not sure what's going on. I added support for `Py_LegacyWindowsStdioFlag` to PyOxidizer a few hours ago. I'd start there. Feel free to make noise on PyOxidizer's issue tracker!
  
  That did the trick, and now I've got a usable (so far) oxidized binary.
  
  What do you think about this in `hg debuginstall`:
  
      File "mercurial.debugcommands", line 1486, in debuginstall
        os.path.dirname(pycompat.fsencode(os.__file__)),
    AttributeError: module 'os' has no attribute '__file__'
  
  That's not really a resource, so just special case it to print the exe?  (This is trying to print the equivalent of this: `checking Python lib (C:\Program Files\Python37\lib)...`)  There's also similar not-quite-resources use of __file__ in service of `hg extensions`, for example.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7444/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7444

To: mharbison72, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Nov. 17, 2019, 2:09 a.m.
indygreg added a comment.


  PyOxidizer doesn't set `__file__` for modules imported from memory. You can work around by installing the Python standard library in an app-relative directory (like we do for the Mercurial modules in my proof-of-concept PyOxidizer patch) or you can teach Mercurial to react gracefully when `__file__` isn't defined. The `debuginstall` use of `__file__` is pretty dubious and not critical to behavior, so I highly favor making it behave better when `__file__` is missing.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7444/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7444

To: mharbison72, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel

Patch

diff --git a/setup.py b/setup.py
--- a/setup.py
+++ b/setup.py
@@ -713,30 +713,34 @@ 
             self.compiler.compiler_so = self.compiler.compiler  # no -mdll
             self.compiler.dll_libraries = []  # no -lmsrvc90
 
-        # Different Python installs can have different Python library
-        # names. e.g. the official CPython distribution uses pythonXY.dll
-        # and MinGW uses libpythonX.Y.dll.
-        _kernel32 = ctypes.windll.kernel32
-        _kernel32.GetModuleFileNameA.argtypes = [
-            ctypes.c_void_p,
-            ctypes.c_void_p,
-            ctypes.c_ulong,
-        ]
-        _kernel32.GetModuleFileNameA.restype = ctypes.c_ulong
-        size = 1000
-        buf = ctypes.create_string_buffer(size + 1)
-        filelen = _kernel32.GetModuleFileNameA(
-            sys.dllhandle, ctypes.byref(buf), size
-        )
+        pythonlib = None
 
-        if filelen > 0 and filelen != size:
-            dllbasename = os.path.basename(buf.value)
-            if not dllbasename.lower().endswith(b'.dll'):
-                raise SystemExit(
-                    'Python DLL does not end with .dll: %s' % dllbasename
-                )
-            pythonlib = dllbasename[:-4]
-        else:
+        if getattr(sys, 'dllhandle', None):
+            # Different Python installs can have different Python library
+            # names. e.g. the official CPython distribution uses pythonXY.dll
+            # and MinGW uses libpythonX.Y.dll.
+            _kernel32 = ctypes.windll.kernel32
+            _kernel32.GetModuleFileNameA.argtypes = [
+                ctypes.c_void_p,
+                ctypes.c_void_p,
+                ctypes.c_ulong,
+            ]
+            _kernel32.GetModuleFileNameA.restype = ctypes.c_ulong
+            size = 1000
+            buf = ctypes.create_string_buffer(size + 1)
+            filelen = _kernel32.GetModuleFileNameA(
+                sys.dllhandle, ctypes.byref(buf), size
+            )
+
+            if filelen > 0 and filelen != size:
+                dllbasename = os.path.basename(buf.value)
+                if not dllbasename.lower().endswith(b'.dll'):
+                    raise SystemExit(
+                        'Python DLL does not end with .dll: %s' % dllbasename
+                    )
+                pythonlib = dllbasename[:-4]
+
+        if not pythonlib:
             log.warn(
                 'could not determine Python DLL filename; assuming pythonXY'
             )