Fix path separator on Windows distributions #275
Conversation
|
We require contributors to sign our Contributor License Agreement and we don't have one on file for @andysham. In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#793), and ping the bot to refresh the PR. |
conda-bot
added
the
cla-signed
|
The failing test on (macos-latest, 3.8) does not seem related to the change introduced in this PR, it also appears in #271. |
|
@jezdez @dbast @kenodegard @jaimergp |
| update_prefix(os.path.join(new_prefix, path), new_prefix, | ||
| placeholder, mode=mode) | ||
| new_path = os.path.join(new_prefix, path) | ||
| if on_win: |
There was a problem hiding this comment.
| if on_win: | |
| if on_win and mode == 'text': |
Do we need the same construct here too?
There was a problem hiding this comment.
I tried this, and found some instances that raised the same error, so I believe now that the issue is more general.
There was a problem hiding this comment.
In that case should the other instance of this if clause be adjusted to not have the mode == 'text' check?
There was a problem hiding this comment.
Perhaps, although given the failure is rising in particular from open(...), where new_prefix (from this scope) is never used, I would suggest these conversions serve different purposes - I would imagine altering the representation of Windows filepaths to Linux filepaths isn't as well supported in Windows binary applications as in Python. Regardless, I feel more comfortable making the minimal change, and the above change doesn't propagate further than the current Python process.
|
Ugh, this is a bit annoying - that test is part of the "required" test suite and I can't override 😓 We'll have to skip or xfail that particular one. |
|
Raised issue at #276 |
|
@jaimergp Thanks for helping me get this merged! What would be required before a |
|
I am going to see if there are some "easy wins" in the PR list and try to bundle a few ones together, but let's aim for "whatever we have in main next week". Does that sound reasonable? |
|
More than reasonable. Thanks for the help! |
Description
I'm experiencing a bug that doesn't allow me to unpack environments on Windows machines, the error presents like so
This reproduction is quite minimal, and it seems to affect all packed files with
mode="text", so I can only assume it's caused by a mismatching path separator - the prefix is defined here using the Windows separator\, and the suffix is modified here to use the separator/, resulting in the mismatch. This PR also modifies the prefix in the same manner, so that the entire path uses the separator/in these cases.Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?Add / update necessary tests?Add / update outdated documentation?