Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: restrict the __config__ modifications to win32 #10339

Merged
merged 1 commit into from Jan 11, 2018

Conversation

Projects
None yet
3 participants
@ghost
Copy link

commented Jan 7, 2018

It seems that sadly 3051ccf had the unintended side-effect of exposing buggy code in that such code should only be run on win32.

extra_dll_dir = os.path.join(os.path.dirname(__file__), '.libs')
if os.path.isdir(extra_dll_dir):
if sys.platform == 'win32' and os.path.isdir(extra_dll_dir):
os.environ["PATH"] += os.pathsep + extra_dll_dir

This comment has been minimized.

Copy link
@eric-wieser

eric-wieser Jan 7, 2018

Member

Wouldn't it be better do the right thing even when PATH is not set? So:

os.environ["PATH"] = os.pathsep.join(os.environ.get("PATH", "").split(os.pathsep) + [extra_dll_dir])
@ghost

This comment has been minimized.

Copy link
Author

commented Jan 7, 2018

Note: this needs a forwardport.

if os.path.isdir(extra_dll_dir):
os.environ["PATH"] += os.pathsep + extra_dll_dir
if sys.platform == 'win32' and os.path.isdir(extra_dll_dir):
os.environ["PATH"] = os.pathsep.join(os.environ.get("PATH", "").split(os.pathsep) + [extra_dll_dir])

This comment has been minimized.

Copy link
@charris

charris Jan 7, 2018

Member

100 char > 79 char. Why do you split the path?

This comment has been minimized.

Copy link
@charris

charris Jan 7, 2018

Member

Note that with the refault path "", you still end up with a os.pathsep prefixing the extra_dll_dir. Is that OK?

This comment has been minimized.

Copy link
@ghost

ghost Jan 7, 2018

Author

Suggested by @eric-wieser. I think I like the previous version better.

This comment has been minimized.

Copy link
@eric-wieser

eric-wieser Jan 8, 2018

Member

Why do you split the path?

An attempt to not end up with that extra pathsep. I thought that "".split(":") would return the empty list, but it does not.

BUG: restrict the __config__ modifications to win32
It seems that sadly 3051ccf had the unintended
side-effect of exposing buggy code that should only be
run on win32.
@ghost

This comment has been minimized.

Copy link
Author

commented Jan 7, 2018

I think that's okay?

dhermes added a commit to dhermes/bezier that referenced this pull request Jan 8, 2018

@eric-wieser

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

I don't think you meant to link to 3051ccf, which doesn't touch the conditional.

Possibly also of relevance:

3cb0e8f#diff-59c1de52780b698ea26e0cbbda5c951cL2298

@ghost

This comment has been minimized.

Copy link
Author

commented Jan 9, 2018

Yes, I did. I thought it was only a name change, so the change would not matter, but it exposed the assumption that PATH is always present to all platforms rather than just win32, leading to the bug report. The change itself wasn't a bad change, but ideally I would have fixed this line as well in that PR.

@ghost

This comment has been minimized.

Copy link
Author

commented Jan 9, 2018

FWIW, it just never occurred to me that PATH would not be set.

@charris charris merged commit 0dc1e70 into numpy:maintenance/1.14.x Jan 11, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@charris

This comment has been minimized.

Copy link
Member

commented Jan 11, 2018

Thanks @xoviat.

@ghost ghost deleted the restrict-config-changes branch Jan 11, 2018

@ghost

This comment has been minimized.

Copy link
Author

commented Jan 11, 2018

Hopefully this is the last that we will need to modify this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.