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

Fixes to support PREFIX containing spaces #449

Merged
merged 83 commits into from
Jan 10, 2023
Merged

Conversation

nsoranzo
Copy link
Contributor

@nsoranzo nsoranzo commented May 7, 2021

{ info added by @jaimergp; original description was empty }

I'll discuss the main points @nsoranzo and I are addressing here. There are two parts: the implementation itself, and then how we managed to test this reliably.

Implementation:

  • @nsoranzo found all instances where PREFIX or derivatives might need quoting to escape spaces in all the installer types.
  • However we still need to deal with the "shebang problem" on entry points and other executables. Spaces are known to break the Unix shebang, but there are some workarounds as hinted by Nicola in this comment. This needs to be done at the conda/conda level. I submitted Better support prefixes with spaces conda#11676 as a result. Note this has hasn't been merged yet, and we should possibly consider it a release blocker (but not a merge blocker), which requires updating the minimum conda-standalone to a version not yet released (4.15?).
  • In Windows, spaces were generally supported because the shebang problem doesn't exist there, but by default the installer still prevented the path from containing spaces. The config key check_path_spaces (defaults to True) was a Windows-only keyword to change this behaviour. We have now extended this key to all platforms and installers, with a default value of True. That means that spaces will not be allowed by default, but a user can opt in by setting it to False.

Tests:

Moved to #560


This would close ContinuumIO/anaconda-issues#716

@anaconda-issue-bot anaconda-issue-bot added the cla-signed [bot] added once the contributor has signed the CLA label May 7, 2021
traversaro added a commit to robotology/robotology-superbuild that referenced this pull request Jun 2, 2021
On Windows, add quotes to correctly invoke conda in path with spaces. 
On Linux/macOS, print a clear error until conda/constructor#449 is fixed.
@traversaro
Copy link

@chenghlee @mcg1969 If there is anything that is necessary or helpful (such as specific tests) to merge this PR, feel free to let me know, I am quite interested in this, thanks!

@mcg1969
Copy link
Contributor

mcg1969 commented Dec 29, 2021

Sorry for the delay in reviewing. But I can't seem to get this to work on a Mac.

For one thing, it seems that the changes were attempted only for the case where the prefix is offered on the command line; that is, <script> -p <install_path>. On the other hand, if you install this in interactive mode, and you supply your desired prefix on the command line, it still rejects it if it contains spaces.

Furthermore, on the Mac, the -p option isn't working. It seems like bash is insisting on breaking apart the prefix, even if I quote it, during the getopt parsing. I can see you tried to change the getopt command to fix that, but I think that on the Mac it's not doing what you expect.

Here's the good news. I modifying the header so that it skips the space checking for interactive input—basically collapsed that entire case statement to PREFIX="$user_prefix". (Note that I removed the eval; that was important.) I was able to build an installer that successfully allowed itself to install into a prefix with spaces, and the environment activated as I would expect. So this is, in theory, a valid approach, it just looks like it needs more work!

Nicogene pushed a commit to Nicogene/robotology-superbuild that referenced this pull request Feb 14, 2022
On Windows, add quotes to correctly invoke conda in path with spaces. 
On Linux/macOS, print a clear error until conda/constructor#449 is fixed.
@nsoranzo
Copy link
Contributor Author

nsoranzo commented May 4, 2022

Sorry for the delay in reviewing.

@mcg1969 Thanks for the review, and sorry for the delay in coming back to this!

But I can't seem to get this to work on a Mac.
For one thing, it seems that the changes were attempted only for the case where the prefix is offered on the command line; that is, <script> -p <install_path>. On the other hand, if you install this in interactive mode, and you supply your desired prefix on the command line, it still rejects it if it contains spaces.

Furthermore, on the Mac, the -p option isn't working. It seems like bash is insisting on breaking apart the prefix, even if I quote it, during the getopt parsing. I can see you tried to change the getopt command to fix that, but I think that on the Mac it's not doing what you expect.

I didn't have a Mac to test, but I believe the issue is that getopt (as installed by default on macOS) is a different and older version w.r.t. the enhanced one available on Linux, and it's just not possible to parse options with spaces with that version.
Therefore, I've simply removed the use of getopt and used the existing getopts fallback which should work on both systems.

Here's the good news. I modifying the header so that it skips the space checking for interactive input—basically collapsed that entire case statement to PREFIX="$user_prefix". (Note that I removed the eval; that was important.) I was able to build an installer that successfully allowed itself to install into a prefix with spaces, and the environment activated as I would expect. So this is, in theory, a valid approach, it just looks like it needs more work!

Thanks for tip, I've adopted it in the rebase.

I've now tested both the batch and interactive mode, and both works with this updated patch.

N.B: The installed conda environment have issues with the bin/conda Python script, since the shebangs interpreter path cannot contain spaces, but this can be fixed as virtualenv has done:

#!/bin/sh
'''exec' "/tmp/venv with spaces/bin/python" "$0" "$@"
' '''

But that's for another day and repository.

@jaimergp
Copy link
Contributor

jaimergp commented Aug 1, 2022

xref conda/conda#186

@jaimergp
Copy link
Contributor

jaimergp commented Aug 1, 2022

Right now, conda does not allow spaces in environments, but it can be worked around with explicit paths.

This doesn't work:

$ conda create -n "with spaces" python pip

CondaValueError: Invalid environment name: 'with spaces'
  Characters not allowed: ('/', ' ', ':', '#')

This does work:

$ conda create -p "./with spaces" python pip
WARNING: A space was detected in your requested environment path
'/Users/jrodriguez/tmp/with spaces'
Spaces in paths can sometimes be problematic.

# proceeds as normal

However, once you need to run an entry point executable (or any script with a shebang), problems arise, as expected:

$ conda activate "./with spaces"
$ pip
zsh: /Users/jrodriguez/tmp/with spaces/bin/pip: bad interpreter: /Users/jrodriguez/tmp/with: no such file or directory

I'll see if I can get your virtualenv-inspired solution on upstream conda.

@jaimergp
Copy link
Contributor

jaimergp commented Aug 1, 2022

See conda/conda#11676

@nsoranzo nsoranzo requested a review from a team as a code owner August 15, 2022 09:07
@jaimergp
Copy link
Contributor

conda/conda#11676 hasn't landed yet but I am tidying up here and making sure I understand the changes. Sorry about the getopt[s] noise 😬

@nsoranzo
Copy link
Contributor Author

@jaimergp Thanks for pushing this forward here and on the conda repo, I've added a small follow-up commit.

@jaimergp
Copy link
Contributor

jaimergp commented Jan 9, 2023

I just realized that spaces might break a properly initialized conda in base (even with latest versions). So, for now, I suggest we:

However this PR would allow spaces in prefix for apps that only want to deploy something without conda, so it's still super useful! In the future, if conda can be initialized to a prefix with spaces safely (in some systems like CentOS 7, the "working" approach would drop PS1, see conda/conda#11885), then we can remove the safeguards.

@jaimergp
Copy link
Contributor

jaimergp commented Jan 9, 2023

@kenodegard I think there's a Github UI bug and I can't see anymore what changes you were requesting, but I am guessing they are now addressed (hopefully! 😂). See last comment for a summary, but happy to answer any other questions.

dbast
dbast previously approved these changes Jan 9, 2023
dbast
dbast previously approved these changes Jan 10, 2023
@jaimergp jaimergp requested a review from kenodegard January 10, 2023 14:43
@kenodegard kenodegard removed their request for review January 10, 2023 14:59
kenodegard
kenodegard previously approved these changes Jan 10, 2023
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise 🚀

.github/workflows/main.yml Outdated Show resolved Hide resolved
@dbast dbast dismissed stale reviews from kenodegard and themself via c57b9ff January 10, 2023 16:08
@jaimergp
Copy link
Contributor

Looks like it was not needed after all :D

@jaimergp
Copy link
Contributor

Oh our dear PR, your time has come! You can now join main 🚀

@jaimergp jaimergp merged commit 2d9062b into conda:main Jan 10, 2023
@nsoranzo nsoranzo deleted the patch-1 branch January 10, 2023 16:58
@jaimergp
Copy link
Contributor

Thank you @nsoranzo and sorry for the longest review ever :D

@nsoranzo
Copy link
Contributor Author

Thank @jaimergp and all contributors for the work on this! 🎉

@jezdez
Copy link
Member

jezdez commented Jan 12, 2023

Awesome work everyone!

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Jan 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support spaces in the install path for the installers
9 participants