zerothi comments#153
Conversation
- REPO_BASE_URL -> PS_REPO_URL - All script-local variables are lowercase
| ARCH="$(uname -m)" | ||
| INSTALLER_NAME="Miniforge3-MacOSX-${ARCH}.sh" | ||
| INSTALL_DIR="$HOME/miniforge3-dtu" | ||
| base_url="https://github.com/philipnickel/miniforge-PIS/releases/latest/download" |
There was a problem hiding this comment.
I think our base_url should be an argument or be allowed to be overwritten?
In this way we can also use it for testing.
There was a problem hiding this comment.
Additional comment: I don't really like the network of interdependency that exists because of PS_REPO_URL being inherited throughout scripts that call each other. For example, install_all_macOS.sh depends on Core/VsCode/install/install_macOS.sh, which depends on settings_macOS.sh and extensions_macOS.sh, which depend respectively on default_settings_MacOS.json and extensions.txt. This will be a major source of complexity if the project grows.
I think it would be better if each script was fully independent, but this would require pasting the extensions and settings in the installation scripts. Further, Core/VsCode/install/install_macOS.sh would only install VS Code and nothing else. This would isolate the scripts from each other (single responsibility principle etc.) and make them easier to test. PS_REPO_URL would only be used in the orchestration install_all_macOS.sh script
| REPO_BASE_URL="${REPO_BASE_URL:-https://raw.githubusercontent.com/dtudk/pythonsupport-scripts/dev}" | ||
| export REPO_BASE_URL | ||
| PS_REPO_URL="${PS_REPO_URL:-https://raw.githubusercontent.com/dtudk/pythonsupport-scripts/dev}" | ||
| export PS_REPO_URL |
There was a problem hiding this comment.
Do we need the export, are we sourcing other scripts? If this gets executed the env won't be shared to the caller.
There was a problem hiding this comment.
We are not currently sourcing the scripts, instead running them in child shells, so the export is necessary. Sourcing them would be a bit dangerous because variables/functions etc. could overwrite each other if we are not very careful.
| set -euo pipefail | ||
|
|
||
| CODE_CLI="/Applications/Visual Studio Code.app/Contents/Resources/app/bin/code" | ||
| std_code_cli="/Applications/Visual Studio Code.app/Contents/Resources/app/bin/code" |
There was a problem hiding this comment.
AFAIK the code executable will only be available once one executes the Install code command in VSCode, will this path always be there? And is the directory always the same?
There was a problem hiding this comment.
Apparently, there is a command on macos called mdfind which would allow us to find the VSC bundle on the system using the Spotlight machinery. Something like this might work:
code_app="$(mdfind "kMDItemCFBundleIdentifier == 'com.microsoft.VSCode'" | head -n 1)"
code_cli="$code_app/Contents/Resources/app/bin/code"I can't really test this since I don't have a MacOS machine, but maybe this is something @philipnickel could look into.
|
|
||
| SETTINGS_DIR="$HOME/Library/Application Support/Code/User" | ||
| SETTINGS_FILE="$SETTINGS_DIR/settings.json" | ||
| settings_dir="$HOME/Library/Application Support/Code/User" |
There was a problem hiding this comment.
Are there other settings directories that VS Code might use? I.e. $HOME/.config or similar?
| TMPDIR_PATH="$(mktemp -d)" | ||
| trap 'rm -rf "$TMPDIR_PATH"' EXIT | ||
| tmpdir_path="$(mktemp -d)" | ||
| trap 'rm -rf "$tmpdir_path"' EXIT |
There was a problem hiding this comment.
fail, transfer ' -> " and vice-versa, add KILL INT
zerothi
left a comment
zerothi
left a comment
There was a problem hiding this comment.
I think we're almost there.
2 participants
Addresses comments in #137.
Regarding Philip's comment, warns and exits if a settings.json already exists.