pkg installer: Fix ownership of files installed into $HOME #784
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
conda-bot
added
the
cla-signed
marcoesters
changed the title
jaimergp
reviewed
May 8, 2024
| # install location, the permissions will default to root unless this is done. | ||
| chown -R "$USER" "$PREFIX" | ||
| chown -R "$USER" "${HOME}/.conda" |
There was a problem hiding this comment.
I think it might also leave a .condarc somewhere if the config file asked for that option. Should we add it here too? e.g.
Suggested change
| chown -R "$USER" "${HOME}/.conda" | |
| test -f "${HOME}/.condarc" && chown "$USER" "${HOME}/.condarc" |
There was a problem hiding this comment.
When I tested it, .condarc had the correct ownership, but better safe than sorry.
jaimergp
reviewed
May 8, 2024
constructor/osx/update_path.sh
Outdated
Comment on lines
19
to
42
| if [[ "${USER}" != "root" ]]; then | ||
| echo "Fixing permissions..." | ||
| read -r -a MODIFIED_FILES <<< "$(\ | ||
| echo "${INIT_FILES}" |\ | ||
| grep modified |\ | ||
| awk '{print $2}' |\ | ||
| # Only grab files inside $HOME or $PREFIX. | ||
| # All init files should be there, but that may change, and it | ||
| # is better to miss files than to have an infinite loop below. | ||
| grep -E "^(${HOME}|${PREFIX})"\ | ||
| )" | ||
| for file in "${MODIFIED_FILES[@]}"; do | ||
| while [[ "${file}" != "${HOME}" ]] && [[ "${file}" != "${PREFIX}" ]]; do | ||
| # Check just in case the file wasn't created due to flaky conda init | ||
| if [[ -f "${file}" ]] || [[ -d "${file}" ]]; then | ||
| OWNER=$(stat -f "%u" "${file}" | id -un) | ||
| if [[ "${OWNER}" == "root" ]]; then | ||
| chown "${USER}" "${file}" | ||
| fi | ||
| fi | ||
| file="${file%/*}" | ||
| done | ||
| done | ||
| fi |
There was a problem hiding this comment.
Awesome shell skills here! This script is called with sh though, so I think we need to either:
- Change the shebang to bash
- Make sure we only use strict POSIX (e.g. no
[[ ]]ifs).
There was a problem hiding this comment.
Doesn't sh link to bash on MacOS?
There was a problem hiding this comment.
Even if it does, I think it's better to be explicit. I changed the shell to bash since that's what we're using in the other scripts as well.
jaimergp
reviewed
May 8, 2024
jaimergp
reviewed
May 8, 2024
|
Let's double check SH vs BASH vs POSIX for that nice chunk of shell code :) |
Co-authored-by: jaimergp <[email protected]>
Co-authored-by: jaimergp <[email protected]>
…ctor into pkg-file-permissions
jaimergp
approved these changes
May 8, 2024
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When installing outside
$HOME, the pkg installer installs asroot. Constructor changes the ownership of the$PREFIXdirectory to$USER:constructor/constructor/osx/run_installation.sh
Line 115 in 58385ee
However, the installer creates additional files and directories outside of
$PREFIX:${HOME}/.condaconda init, e.g.,${HOME}/.bash_profileor${HOME}/.config/powershellThis PR fixes the ownership of those files and directories as well.
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?