Closed jaimergp closed 3 years ago
Hi! This is the friendly automated conda-forge-linting service.
I just wanted to let you know that I linted all conda-recipes in your PR (recipe
) and found it was in an excellent condition.
Thanks for looking into this Jaime! 😄
Unfortunately it appears we are getting build failures on Windows as well. Maybe related to the comment above?
Seems like the change does indeed quote the path (this is what we need, I think), but that messes up the tests because quotes in quotes is a nono in batch. I'll have to come up with a different way to test it then...
Not sure which tests are failing, but just wanted to throw in (probably you know already?) that if you want to get rid of quotes at variable insertion time, you could use %MY_VAR%:"=%
I'll leave it here for today. At some point batch just becomes a set of little puzzles 🥳 /sarcasm
After some local debugging, turns out it was easier than whatever I was trying to do.
Since unsetting a variable in cmd involves set "VARIABLE="
, we don't need to test for the backup variable existence: if it has a value it will be set to that; if not it will be functionally unset.
I have also enabled @echo on
on the activation script.
A question remains though: do we really need CFLAGS on Windows? I guess it's only for the GCC compilers (msys2?) but is that even supported with CUDA? Also, what about INCLUDE
and friends for MSVC? Should I be setting something else?
Thanks @jaimergp!
I would like to give this PR a whirl in conda-forge/faiss-split-feedstock#19, so I'd prefer if we can defer the question about CFLAGS for afterwards. :)
A question remains though: do we really need CFLAGS on Windows?
Only CMake uses them AFAIK
I guess it's only for the GCC compilers (msys2?) but is that even supported with CUDA?
No, gcc compilers don't use the flags, nor any other compiler for that matter. It's used by build systems only
Also, what about INCLUDE and friends for MSVC? Should I be setting something else?
INCLUDE is used by cl.exe and by CMake which passes it to cl.exe and nvcc. So, we should set INCLUDE only.
I ran nvcc --verbose demo.cu
without setting INCLUDE
manually and it apparently this is handled automatically? Do you think we still need it?
Could we have one build with the flags-path fixes before figuring out whether CFLAGS
& INCLUDE
is absolutely necessary? 🙃
I am ok with merging this one and dealing (or not) with INCLUDE later. We need one more maintainer to agree though :)
I guess that means we are merging! I'll leave it here for one more hour just in case somebody wants to say otherwise.
There we go!
Thanks Jaime for the PR and everyone for the reviews! 😄
Checklist
0
(if the version changed)conda-smithy
(Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)Trying to fix https://github.com/conda-forge/faiss-split-feedstock/pull/19#issuecomment-743106403
We'll probably need to add more tests here to cover these cases, because this wasn't caught earlier :/