Skip to content

Conversation

@Klaim
Copy link
Member

@Klaim Klaim commented Aug 5, 2022

Also adds tests checking this issue in the first commit. They fail for me on Windows 11 without the fix provided in the second commit.

I believe this should fix #1805 which was re-opened after #1828 re-introduced the issue.

@Klaim
Copy link
Member Author

Klaim commented Aug 5, 2022

@chaubold pinged you as this should fix #1805 that was a regression from #1828

@Klaim Klaim force-pushed the klaim/fix/install-pip-whitespace-prefix branch from 4159bf3 to e9ee5a2 Compare August 5, 2022 16:10
@chaubold
Copy link
Contributor

chaubold commented Aug 5, 2022

Preventing the argument splitting by creating a vector<string> right away makes a lot of sense to me, thanks.

I have built it locally and tried the same use case that gave rise to my comment in #1828 and to reopening the issue. With the changes in this PR, everything works fine 👍 Nice one!

@Klaim Klaim force-pushed the klaim/fix/install-pip-whitespace-prefix branch from e9ee5a2 to 39c814f Compare August 6, 2022 13:20
@jonashaag
Copy link
Contributor

Does this also apply if we have a environment name with a space in environment.yml? (Is that even allowed?) If yes, shall we add a test as well?

@Klaim
Copy link
Member Author

Klaim commented Aug 8, 2022

Does this also apply if we have a environment name with a space in environment.yml? (Is that even allowed?) If yes, shall we add a test as well?

I didn't consider that case, I'll add a test.

@Klaim
Copy link
Member Author

Klaim commented Aug 8, 2022

@jonashaag For some unclear reason your changes don't pass for me (windows 11) because the type of prefix is WindowsPath('...') and can't be passed as is to that function so f"{prefix}" is used as a conversion to string. I'm not sure why it passes on CI, but maybe it's a python version mismatch.

@Klaim
Copy link
Member Author

Klaim commented Aug 8, 2022

The initial fix should cover any prefix value provided (as long as it have been correctly parsed before reaching that part of the code). I added the test.

@wolfv
Copy link
Member

wolfv commented Aug 8, 2022

Thanks @Klaim !

@wolfv wolfv merged commit 0b8b9a7 into mamba-org:master Aug 8, 2022
@jonashaag
Copy link
Contributor

@jonashaag For some unclear reason your changes don't pass for me (windows 11) because the type of prefix is WindowsPath('...') and can't be passed as is to that function so f"{prefix}" is used as a conversion to string. I'm not sure why it passes on CI, but maybe it's a python version mismatch.

The proper way would be to use str.

@chaubold
Copy link
Contributor

chaubold commented Aug 9, 2022

Are there any plans to build a 0.25.2 release with this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[micromamba] micromamba create fails to install pip dependencies if prefix contains whitespace (Windows)

4 participants