FIX Remove temporary filenames from command list. #20220
Conversation
There was a problem hiding this comment.
Thanks @cmarmo.
When we merge this we should open another issue so that we can test against this to avoid breaking the build instructions again.
setup.py
Outdated
| @@ -265,7 +265,8 @@ def setup_package(): | |||
| package_data={'': ['*.pxd']}, | |||
| **extra_setuptools_args) | |||
|
|
|||
| commands = [arg for arg in sys.argv[1:] if not arg.startswith('-')] | |||
| commands = [arg for arg in sys.argv[1:] if not arg.startswith('-') and | |||
| not arg.startswith('/')] | |||
There was a problem hiding this comment.
Does the / come from a file-path or something like that? Will this also work on Windows where the file separator is \?
Eitherway it would be good to add a small comment
There was a problem hiding this comment.
On Windows filenames would even typically start with r"C:\\", r"D:\\" to r"Z:\\" as often used for shared folders on virtual machines... (I guess r"A:\\" and r"B:\\" also would be possible). And maybe r"\\" as well but I am not 100% sure.
There was a problem hiding this comment.
Right sorry, I'm not very good at Windows fixes... This will definitely not work with Windows. I will try harder.
Before #19650 setup.py command line was parsed: if the argument was one in the list then the action was required to succeed without Numpy. After #19650 the list is built excluding arguments starting with |
|
After deeper reading of the traceback So I need to check if this is a directory... |
| commands = [arg for arg in sys.argv[1:] if not arg.startswith('-')] | ||
| # Exclude from the list of arguments options and temporary directories | ||
| commands = [arg for arg in sys.argv[1:] if not arg.startswith('-') and | ||
| not os.path.isdir(arg)] |
There was a problem hiding this comment.
I am worried about the extra system calls and the facts that we might have other kinds of command arguments that are not file that we should neither treat as commands themselves.
|
Maybe it would be more robust to do the opposite and explicitly list commands that require numpy. I assume they are
|
But then you just might want revert #19650? |
|
I think it's worth trying to implement the strategy described in #20220 (comment) in a dedicated PR and run |
Reference Issues/PRs
Fixes #20212
What does this implement/fix? Explain your changes.
#19650 did not exclude temporary filenames from the argument list.
Any other comments?
ping @NicolasHug ? Thanks!