Fixes to support PREFIX containing spaces

[nsoranzo]

{ 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 https://github.com/conda/conda/pull/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 https://github.com/ContinuumIO/anaconda-issues/issues/716

[traversaro]

@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]

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!

[nsoranzo]

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]

xref https://github.com/conda/conda/issues/186

[jaimergp]

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]

See https://github.com/conda/conda/pull/11676

[jaimergp]

https://github.com/conda/conda/pull/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]

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

[jaimergp]

@nsoranzo Good catch! It made me think of the OSX scripts too, where I quoted some more stuff.

I also brought back the spaces checking code, but controlled with the existing check_path_spaces key. This key was only available on Windows before, but I have now extended its usage to all installer types. It defaults to false now.

Let me know what you think!

[jaimergp]

Windows checks have been passing all along, and they should have failed with the last commit (Python versions in extra_envs example are different now, but I didn't update the post_install.bat script). I think this is fixed by #553, so let's wait til that's merged before making a decision here.

[nsoranzo]

I think this is fixed by #553, so let's wait till that's merged before making a decision here.

#553 is already merged, just need to merge main here?

[jaimergp]

There's an issue in silent mode, where /D is ignored if it contains spaces o.o, falling back to the default location. That's why the install.log cannot be found. On top of that, post_install scripts do not error out properly on NSIS (maybe by design?). I am adding an environment variable to control that.

[jaimergp]

Ok now we should still see the errors on Windows, but this time because we intercepted the failures properly. Let's see!

[jaimergp]

Excellent, post-install script fails as expected for commit 9b6e459. This is the error we see on CI:

$ constructor D:\a\constructor\constructor\examples\extra_envs --output-dir C:\Users\RUNNER~1\AppData\Local\Temp\tmpnwnycwb0\tmp5l52cjz2
--- STDOUT ---
---- testing ExtraEnvs-X-Windows-x86_64.exe
cmd.exe /c start /wait C:\Users\RUNNER~1\AppData\Local\Temp\tmpnwnycwb0\tmp5l52cjz2\ExtraEnvs-X-Windows-x86_64.exe /S /D=C:\Users\RUNNER~1\AppData\Local\Temp\tmpnwnycwb0\tmp5l52cjz2\tmpf7hk9_dls p a c e s
--- STDOUT ---
---  LOGS  ---
detailprint: ::error:: Failed to run post install script

Now I will fix the post-install script so it passes properly.

[jaimergp]

There we go! I think we are ready for a review here.

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 this PR 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:

We then had to check whether everything worked as intended or there were other gotchas in the way. To do so, we changed the tests to always install to a path that contained spaces. This led us to discover some issues with the Windows tests:

• The NSIS installers always exit with a 0 exit code, regardless it worked or failed. Not a good test 😬
• The silent mode /S on Windows is REALLY silent. Nothing (and I mean nothing) gets printed to stdout or stderr. If something fails, it generally keeps going on (the error dialogs are configured to "ignore" in silent mode; check the /SD parameter next to each MessageBox).
• To make things worse, the conda install command was not reporting its possible errors via a dialog, so I changed this to throw an error message that aborts the installation. Again, remember, none of these errors get reported to stdout or stderr.
• What if we manually print to a file then? Enabling log files with some of the output required using a special build for NSIS, so I am patching our CI installation process to do so. We then slightly modified our error messages to always include the ::error:: sentinel value, which we can grep out of the logfile after the installation has concluded. If we find this sentinel value, then we tell run_examples.py to report that installation as failed.
• Note that these errors won't include the output from the failing command. That output will only appear on the GUI logs due to nsExec limitations. In other words, CI will only report whether the error occurred, but further debugging needs to be done locally on a Windows machine with desktop and graphical installer.
• Another note: errors due to path issues (spaces if enabled, unicode, permissions, etc) won't appear in the logfile because the destination path does not exist yet at that. Hence, a FileNotFoundError could mean an issue with the paths. I have disabled all path checks in our examples so they are guaranteed to work, but if new examples arise without those keys, a user-friendly error will them what to look for.
• Lastly, _nsis.py was not reporting the error code of the scripts it was running (pre-uninstall or post-install). This might have been done on purpose, so I didn't want to change that default behaviour. Instead, it will only report subprocess errors (via exit code) if a special environment variable is set. We are setting it in our CI, since end users might not need it. That said, I am open to changing this conservative decision and leave it as "always tell me if something went wrong".

---

I hope you enjoyed the read. All yours @conda/constructor!

[nsoranzo]

@jaimergp Needs a rebase/merge due to a new conflict, are you taking care of it or should I?

[jaimergp]

I am splitting the Windows test stuff in a different PR for easier review. Once that's merged, I'll rebase here and then we can request a review :) Changing to draft for now.

[jezdez]

I've removed this from the current sprint, since we're still reeling from the effects of the supporting patch in https://github.com/conda/conda/issues/11893.