Skip to content
This repository has been archived by the owner on Feb 15, 2023. It is now read-only.

DLL path checks/fixes #58

Merged
merged 6 commits into from
Nov 20, 2019

Conversation

tylerjereddy
Copy link
Collaborator

@tylerjereddy tylerjereddy commented Nov 18, 2019

Fixes #57 (eventually)

Currently contains:

  • 1 commit to reduce the sizes of the test matrices to improve iteration times (to be reverted when PR is ready)
  • 1 commit to augment one of our current wheels test scripts to verify that all DLLs packaged with SciPy wheels are at the same base path

Still likely needs:

  • perhaps rename check_license.py now that it is doing more than that (suggested by Pauli)
  • confirmation that the msvcp140.dll improper location is actually detected by the new "test"
  • placing msvcp140.dll in the correct/updated DLL basepath location

* reduce the sizes of the matrices
for Travis & appveyor wheels CI runs
to improve debug iteration times
@rgommers
Copy link
Contributor

CI is not happy yet:

Successfully installed scipy-1.5.0.dev0+cf4a407
  File "../check_license.py", line 28
    for filename in Path(install_basedir).rglob('*.dll')

SyntaxError: invalid syntax

* augment check_license.py so that it
verifies that all DLLs packaged with SciPy
wheels reside at the same basepath
@tylerjereddy
Copy link
Collaborator Author

Error message on Windows/Appveyor looks nice now:

python ..\check_license.py
mismatch between current DLL file path:  C:\Python38-x64\lib\site-packages\scipy\extra-dll\msvcp140.dll and the 
reference file path for packaged DLLs:  C:\Python38-x64\lib\site-packages\scipy\.libs

More work needed to only do this on Windows platform & other things noted above.

* restrict the DLL common path checking to Windows
only

* rename check_license.py to check_installed_package.py
as suggested by Pauli to reflect the broader scope of
the module
@tylerjereddy
Copy link
Collaborator Author

The detection of the problem by a "test" in CI looks good now.

I'm iterating on the actual fix now.. since we actually patch over distutils in some cases, using the NumPy version doesn't quite seem safe for making a decision on the DLL storage path.

I've tentatively tried importing the NumPy that is available at wheel build time & using the standard library to parse the source code proper for an indication that .libs is being used, otherwise falling back on extra-dll. I don't know if that is particularly fragile..

@tylerjereddy tylerjereddy force-pushed the issue_57_dll_paths branch 3 times, most recently from b6d0d74 to 9d9135c Compare November 19, 2019 05:07
@pv
Copy link
Contributor

pv commented Nov 19, 2019

I'd maybe just hardcode the DLL path, as it's unlikely Numpy will ever change it from .libs (and the test here will then catch it). More likely changes in the future are from Python.org bumping the msvcrt version, so it's probably better to keep it simple here.

* all DLLs, including msvcp140.dll, should now be placed
at the scipy/.libs path
@tylerjereddy
Copy link
Collaborator Author

Ok, I backed out the last fix & substituted with a fix that simply replaces extra-dll with .libs in your original DLL copy code.

I guess we'll see if CI likes that. Did NumPy ever explicitly use extra-dll, or was that just coming from us, or was it a Python-wide default of some sort?

@rgommers
Copy link
Contributor

I guess we'll see if CI likes that. Did NumPy ever explicitly use extra-dll, or was that just coming from us, or was it a Python-wide default of some sort?

never a Python thing, IIRC we had the same naming and build setup in NumPy and SciPy. Or it was a multibuild thing (which is still us, more or less)

@pv
Copy link
Contributor

pv commented Nov 19, 2019

IIRC, extra-dll was used by us because we pinned numpy-distutils to a prerelease version to get the windows build working ASAP. It was changed a bit later to .libs in numpy before the relevant Numpy release, so Numpy releases always used .libs.

@tylerjereddy
Copy link
Collaborator Author

CI was happy with the reduced test matrix scenario--now restoring the full test matrix with fingers crossed.

@tylerjereddy
Copy link
Collaborator Author

32-bit Linux failures look unrelated -- I've opened an issue

The single Appveyor failure so far does look real & related to my changes, though most entries are passing at least:

python ..\check_installed_package.py
Traceback (most recent call last):
  File "..\check_installed_package.py", line 79, in <module>
    main()
  File "..\check_installed_package.py", line 73, in main
    check_dll_paths(mod)
  File "..\check_installed_package.py", line 35, in check_dll_paths
    reference_basepath = os.path.dirname(list_filepaths.pop(0))
  File "C:\Python35\lib\ntpath.py", line 239, in dirname
    return split(p)[0]
  File "C:\Python35\lib\ntpath.py", line 204, in split
    d, p = splitdrive(p)
  File "C:\Python35\lib\ntpath.py", line 139, in splitdrive
    if len(p) >= 2:
TypeError: object of type 'WindowsPath' has no len()
Command exited with code 1

Most likely the DLL path checking code is not quite portable to Python 3.5 on Windows.

* improve Windows/Python 3.5 compatibility
of the DLL path checking code in
check_installed_package.py
@tylerjereddy
Copy link
Collaborator Author

I reproduced the Windows Python 3.5 issue locally & managed to fix the pathlib issues with some string coercion---let's hope CI agrees.

@tylerjereddy
Copy link
Collaborator Author

Incidentally, check_installed_package.py also works to detect the DLL issue in local pip-installed wheels of SciPy 1.3.2 on Windows.

@tylerjereddy tylerjereddy changed the title WIP: DLL path checks/fixes DLL path checks/fixes Nov 20, 2019
@tylerjereddy
Copy link
Collaborator Author

Windows/Appveyor now all green with DLL adjustments. The 32-bit Linux issues are not related & were briefly discussed separately with Ralf in main repo issue, so in this goes.

@tylerjereddy tylerjereddy merged commit b11f900 into MacPython:master Nov 20, 2019
@tylerjereddy tylerjereddy deleted the issue_57_dll_paths branch April 14, 2020 23:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix msvcp140.dll copying to match Numpy distutils
3 participants