Add MSYS2 path translation exclusion for CL with MSVC builds #900
Conversation
|
Is this meant to fix ( #895 )? |
|
Sorry I'm away this week with work and they are fairly intense, long days. I did have a bit of a look at this and I think it might be to do with delayed expansion within the SDK script. We may need to flip it on before calling it - as is done in conda-forge right now. |
| # Unfortunately, the Windows SDK takes a different command format for | ||
| # the arch selector - debug is default so explicitly set 'Release' | ||
| win_sdk_arch = '/x86 /Release' if bits == 32 else '/x64 /Release' | ||
| win_sdk_arch = '/Release /x86' if bits == 32 else '/Release /x64' |
There was a problem hiding this comment.
Line 97 above. That is documented as being required if the Windows SDK is used. Currently that only gets set if the user overrides the default version - when in fact it may need to be set for VS 2010 only (which is the only version that uses the Win SDK.
There was a problem hiding this comment.
I understand those flags above as telling distutils "stand back, I'm going to try science!"
In other words, it overrides distutils' search for compiler. Maybe I'm wrong - haven't looked deeply at this.
Are there changes you'd suggest here, or just commenting?
|
@jakirkham - hopefully. Testing now with ICU: https://ci.appveyor.com/project/conda-forge/icu-feedstock/build/1.0.17 @patricksnape good idea on delayed expansion. I'll try that next if this doesn't work. |
| @@ -4,7 +4,7 @@ package: | |||
|
|
|||
| source: | |||
| fn: sympy-0.7.5.tar.gz | |||
| url: https://pypi.python.org/packages/source/s/sympy/sympy-0.7.5.tar.gz | |||
| url: https://pypi.python.org/packages/8c/a5/5fa8adee81837687f7315122769fc0b0e8b042c69e2fe5809c41191c7183/sympy-0.7.5.tar.gz | |||
There was a problem hiding this comment.
Unrelated, but it is here. Why do we now have to do this with the URLs?
I have seen this is needed on conda-forge too. Would be nice if we could handle a redirect or whatever so that we can avoid this ugly URL somehow.
cc @ocefpaf
There was a problem hiding this comment.
I don't know when this changed, but pypi changed their web api. They don't seem to offer redirects that I know of. My search terms have been "pypi url change" and stuff like that. Haven't been able to find anything.
There was a problem hiding this comment.
Yeah, I have tried to search for this change and found nothing either, but thought maybe you might know more than I. If you figure out anything, please let me know.
There was a problem hiding this comment.
@msarahan, @jakirkham
Seems to be quite a bit of complaints/discussion related to the pypi URL changes here:
https://bitbucket.org/pypa/pypi/issues/438/backwards-compatible-un-hashed-package
TLDR: doesn't look like it will be fixed on the current pypi, but backwards compatible URLs are featured in "Warehouse" which is PyPI 2.0.
I was able to verify the expected behaviour for the package pyface:
The links listed at https://pypi.io/project/pyface/#files have the hashes in them, but the following old-style URL also works: https://pypi.io/packages/source/p/pyface/pyface-5.1.0.tar.gz
There was a problem hiding this comment.
Awesome, works for sympy too.
https://pypi.io/packages/source/s/sympy/sympy-0.7.5.tar.gz
There was a problem hiding this comment.
I'm going to split that commit off of this PR and make its own. That really needs a fix.
There was a problem hiding this comment.
Was debating asking about just that exactly.
msarahan
force-pushed
the
fix_win_vs2010
branch
from
2da794d
to
49375fa
Compare
msarahan
added
in_progress
Current coverage is 10.4%@@ master #900 diff @@
=========================================
Files 39 41 +2
Lines 5144 5289 +145
Methods 0 0
Messages 0 0
Branches 0 0
=========================================
+ Hits 509 550 +41
- Misses 4635 4739 +104
Partials 0 0
|
msarahan
force-pushed
the
fix_win_vs2010
branch
2 times, most recently
from
9507c93
to
10b09e4
Compare
|
Ultimately, the cause of this issue is that MSYS2 automatically converts things that look like into paths, like C:\root\of\msys2\ai This confuses MSVC a lot. The main additions here are some environment variables that prevent MSYS2's path conversion from acting on a few things. Otherwise, there are some tests that are probably good to have, anyway. |
|
Merging. Passing appveyor build in https://ci.appveyor.com/project/msarahan/conda-build/build/1.0.54 |
msarahan
changed the title
|
Hi there, thank you for your contribution! This pull request has been automatically locked because it has not had recent activity after being closed. Please open a new issue or pull request if needed. Thanks! |
This might work - needs testing. If it doesn't, I think we might need to remove the /Release flag (but this really should work: https://msdn.microsoft.com/en-us/library/ff660764(v=vs.100).aspx)
ping @patricksnape @jakirkham - is there a good way for us to test this with conda-forge?