header.sh: Ensure error mesage is emitted when $HOME is not writable

[dlmiles]

Description

I am not familar with this project's Issue/PR process.

I place this one line change in the public domain and do not require any attribution.

conda/conda#12642
ContinuumIO/anaconda-issues#13189

This patch does not address the other issue in the ContinuumIO/anaconda-issues#13189 of the conda.mk file no restarting by re-running the installer. As it is not clear if that is a conda issue or not.

Checklist - did you ...

Not sure if the change is "significant".
It is a one-liner that has affected things for the past 3 years it seems.

Not sure a test is necessary, I can make one using docker it will takes many minutes to execute and download 100's Mb's in the process just to prove something. Not sure it is really helpful considering the bug has existed for many years already.

I am not aware of any outdated document in respect of this change.

• [*] Add a file to the news directory (using the template) for the next release's release notes?
• [*] Add / update necessary tests?
• [*] Add / update outdated documentation?

[conda-bot]

We require contributors to sign our Contributor License Agreement and we don't have one on file for @dlmiles.

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#763), and ping the bot to refresh the PR.

[dlmiles]

CLA should be completed now. Have updated the commit log message and force push to get CI workflow to complete.

[jaimergp]

If I understand the issue, the behavior is more or less correct (the installation shouldn't continue if ~/.conda could not be created). The problem is that the users suffering from this won't get an error about it, left clueless about the error because of the /dev/null redirection

So what about something like:

$ maybe_error=$(mkdir -p ~/.conda 2>&1 || true)
$ test -d ~/.conda || echo "Could not create ~/.conda: $maybe_error"

Could you check if that works in your setup? Thanks 🙏

[dlmiles]

Yes you are understanding correctly (user left clueless and can think easily the install has completed successfully, and I know of users for years it seems might have done exactly this in their setup). But I do not think your approach to solve it will work.

You said the behaviour is correct (confirming the installer should bail out as a writeable $HOME is a requirement to install).
But the command: echo always returns with exit status 0. So the 2nd line will print something on screen, but not bail out of the installer, because failing_command || succeeding_command is a net success.

There is a lot going on during the install, and a lot of ncurses style line rewrite/redraws, so if the goal was to emit the error but let the installer continue, I think the message will be lost in all the traffic.

Also I don't think you need the || true on the end, I don't think the script will bail if you captured the error code. So you can simply do:

if failing_command
then
another_command
fi

when set -e because you captured the failing_commands error status. The set -e only takes effect for toplevel/naked commands. Maybe test with a shell locally :P

[jaimergp]

Oh, yea I forgot to add an exit 1 after the echo, sorry.

[dlmiles]

Yes that looks like it might work. Except the error message will be to stdout not stderr and the script/line-of-code that the error is from would not be that clear to the user, there are a lot of things run inside that script. Other than this it is better than it was so go for it.

You are aware that the 'mkdir' tool provides a well understood error message all by itself and it probably does not need to be augmented by the message you've chosen.

Anyhow instead of me trying to instil some decades of shell programming experience into you by offering concerns around your creations, do you have a colleague back there who can assist you on this ?
If not maybe buy this book https://www.oreilly.com/library/view/learning-the-bash/0596009658/ I think I must have a previous edition based on the date on the front cover.

Maybe test the patch that has been provided, I know I have tested it, both for the unable to write to $HOME situation and the normal situation writeable situation, and the situation where it needs to create it, and the situation where it already exists.

Maybe you understand the race condition (talked of in the nearby issue links) and why my patch is written like it is.

I've been a good OSS citizen and reported it, provided a patch and signed the CLA. The rest is down to you now.

I don't use this (conda) particular package, I have only come across it while helping the diagnosis of another group's reported situation, that group now have a workaround already in place, but the line of code has been in there for years and appears to have various issues around its existence. It seems to be more complicated than it needs to be.

Good luck!

[jaimergp]

Thank you for the advice! I'll leave it as is but added some comments for the future readers. Let's see how the tests go (I don't anticipate issues) and once the Anaconda folks have checked the CLA this is good to go.

Thanks again for the contribution and discussion!