Skip to content
This repository was archived by the owner on Aug 30, 2024. It is now read-only.

Use 141 toolchain for Windows build #146

Merged
merged 3 commits into from
Jan 26, 2022

Conversation

matthew-brett
Copy link
Contributor

See discussion at #145

I'm afraid I can't test this because I don't have Azure Pipeline permissions
for my own account.

Do I need some permissions from y'all?

@charris
Copy link
Contributor

charris commented Jan 24, 2022

I'm afraid I can't test this because I don't have Azure Pipeline permissions
for my own account.

Test it where? It is running here.

@charris
Copy link
Contributor

charris commented Jan 24, 2022

Linking is failing

MSVCRT.lib(loadcfg.obj) : error LNK2001: unresolved external symbol __enclave_config
_configtest.exe : fatal error LNK1120: 1 unresolved external

@matthew-brett
Copy link
Contributor Author

Sorry - I meant - it is hard to test the code before doing a PR.

@matthew-brett
Copy link
Contributor Author

It looks like the compilation step is failing to find the MS compilers... I'm afraid this is going to mean a whole slew of cruddy debug commits :(

@charris
Copy link
Contributor

charris commented Jan 24, 2022

For a bunch of cruddy test commits it helps to comment out all the other tests, especially travis.

@matthew-brett
Copy link
Contributor Author

@isuruf - any ideas what might be going on here? The error is the one Chuck posted above, repeated here:

MSVCRT.lib(loadcfg.obj) : error LNK2001: unresolved external symbol __enclave_config
_configtest.exe : fatal error LNK1120: 1 unresolved external

The configuration batch output is here - it appears to be finding the 141 toolset and SDK correctly.

I've tried on windows-latest and windows-2019.

I can't replicate it locally. I've forced the same SDK and am using the same toolset locally - and I've run out of things I can think of to try, as of about 1am in the morning. Any ideas what I could try next?

@matthew-brett
Copy link
Contributor Author

OK - I think I worked it out - was missing set MSSdk=1. Ouch.

@charris
Copy link
Contributor

charris commented Jan 25, 2022

The OSX failures started 10 days or so ago. I suspect azure is reponsible somehow, an upgrade from 10.9 to 10.14 may be responsible for the error message, I don't really understand that bit. The windows failures (and errors) are new and look like mostly f2py and may be mingw related.

@isuruf
Copy link
Contributor

isuruf commented Jan 25, 2022

Windows failures are because the testing script is run with MinGW bash which adds it's compilers to the front of PATH and unfortunately there's a link.exe from MinGW toolchain and distutils tries to use that instead of the one from Visual Studio because DISTUTILS_USE_SDK=1

@matthew-brett
Copy link
Contributor Author

What's the right thing to do? We could unset DISTUTILS_USE_SDK before that step- but is that the right fix?

@charris
Copy link
Contributor

charris commented Jan 25, 2022

The OSX failures occur after an azure update to the Python tools version from 0.193 to 0.197, apparently the OSX version changed in the process. See "set python version" tab.

@isuruf
Copy link
Contributor

isuruf commented Jan 25, 2022

We could unset DISTUTILS_USE_SDK before that step- but is that the right fix?

Yeah, that works. It also tests that numpy built with v141 toolchain can successfully link with v142 toolchain.

@matthew-brett
Copy link
Contributor Author

Simple-minded unset didn't seem to work : 5481094

@matthew-brett
Copy link
Contributor Author

@isuruf - thanks - it seems that Bash is not picking up the unset, although it has clearly picked up the earlier variable set (any explanations welcome). I've tried using the ugly Pipeline syntax to change the environment variables.

@charris
Copy link
Contributor

charris commented Jan 26, 2022

I trying to figure out how to give you permissions. If you want to run on your own account, it may be something you can do for yourself.

@matthew-brett
Copy link
Contributor Author

matthew-brett commented Jan 26, 2022

Most unfortunately, Distutils does this check for the relevant variables:

if "DISTUTILS_USE_SDK" in os.environ and "MSSdk" in os.environ and self.find_exe("cl.exe"):

https://github.com/pypa/setuptools/blob/main/setuptools/_distutils/msvccompiler.py#L250

This means that it doesn't matter what values these variables have, including empty strings or 0, it will still trigger effects in Distutils.

So, I have unset the variables in the Bash install / test step. I think that's as neat as we can be, but I'm happy to hear alternatives!

@rgommers
Copy link
Contributor

If it makes life easier (/possible), patching MSVCCompiler.initialize in https://github.com/numpy/numpy/blob/main/numpy/distutils/msvccompiler.py#L43 by copying over the relevant else code and taking out that side effect is fine with me.

@matthew-brett
Copy link
Contributor Author

OK - I think this one is ready to go. Windows tests passing, macOS failures unrelated.

@charris charris merged commit 22584fe into MacPython:main Jan 26, 2022
@charris
Copy link
Contributor

charris commented Jan 26, 2022

Thanks Matthew.

@matthew-brett
Copy link
Contributor Author

Thanks Chuck.

Now - would y'all consider tiny little PRs to build 141 wheels with a build tag for 1.22.0 and 1.22.1 ? That way, Scipy can assume that the builds are 141 compatible, at least for now.

@matthew-brett
Copy link
Contributor Author

Oh - and thanks @isuruf, as ever, for your invaluable help.

@@ -0,0 +1,71 @@
@@echo on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add something like

REM Taken from conda-forge recipes licensed as follows
REM BSD-3-Clause
REM Copyright conda-forge contributors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - see: #150

@rgommers
Copy link
Contributor

Now - would y'all consider tiny little PRs to build 141 wheels with a build tag for 1.22.0 and 1.22.1 ? That way, Scipy can assume that the builds are 141 compatible, at least for now.

We should first make a 1.22.2 release with already merged PRs labelled for backport with this toolchain, there are more critical issues in there for SciPy. Maybe it makes sense to do .post0 releases for 1.20.0/1.20.1, I'm not sure - but it's not all that important either way once 1.22.2 has what we need.

matthew-brett added a commit to matthew-brett/numpy-wheels that referenced this pull request Jan 26, 2022
@mattip
Copy link
Contributor

mattip commented Jan 26, 2022

Maybe we should port this to the numpy/numpy repo and use it in CI

  • to make sure we are testing it
  • which will make it easier to port to the cibuildwheel builds

Other projects may want to use this too

charris pushed a commit that referenced this pull request Jan 26, 2022
charris pushed a commit to charris/numpy-wheels that referenced this pull request Jan 26, 2022
* Use defined image windows-2019

latest image seems like a hostage to fortune.

* Add, use script to set VS toolchain version

* Unset Distutils SDK stuff before tests
charris pushed a commit to charris/numpy-wheels that referenced this pull request Jan 26, 2022
charris added a commit that referenced this pull request Jan 26, 2022
* Update Mac version from 10.9 to 10.14.

This is needed because of changes in the azure provided Python 3.8 for
Mac.

* Use 141 toolchain for Windows build (#146)

* Use defined image windows-2019

latest image seems like a hostage to fortune.

* Add, use script to set VS toolchain version

* Unset Distutils SDK stuff before tests

* Add conda-forge license note (#150)

As suggested by @isuruf in

#146 (comment)

Co-authored-by: Matthew Brett <[email protected]>
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.

5 participants