-
Notifications
You must be signed in to change notification settings - Fork 788
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
feat: install dependencies in .tool-versions order #1723
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited to see this pr! Why even add the option? I'd vote for just making this the unconditional behavior.
I'm down for that, just thought its better to leave the option in there in case it breaks anything for people using the tool now. |
@chrisjpalmer Thanks for this very useful PR + the notes on the tests and the scenarios not handled. Ordered install can be the default behaviour. But I have not given any thought to how we could handle order across |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added notes in the comments but repeating here.
Very useful PR ❤️
- Ordered install can be the default behaviour
- Open to ideas to handle scenarios for situations where there is a .tool-versions in the $HOME and the current directory.
- Tests can be added once we do the above.
Changed this to a draft PR since this is work in progress. |
Thanks Akash. This sounds good. I have some initial thoughts on how we can support multiple Current ProcessBased on some testing and reverse engineering the code, I can see that ASDF does the following steps:
Let me know if you agree with the above analysis. Assuming I'm right, then the key takeaway is that the New ProcessTo emulate the same behaviour whilst now considering the version order in
Just for sanity checking its important to note the following about this new process:
Let me know what you think :) Chris |
Hi there. I have actually gone ahead and implemented this. Current status is:
|
Hey @HashNuke just checking in to see if you are happy with the progress on this PR ? |
UpdateI ran the unit test suite... majority are passing! I added a new unit test which verifies the ordered install behaviour:
Note: these are failing (on my local) but I believe they are unrelated:
For considerationI was chatting with a colleague of mine, and we were thinking that in addition to having tools be installed in .tool-version order, it might be good to consider the directory structure as well when installing tools. Suppose you have the following structure:
Currently in both the previous and new install process, Thoughts on this ? Thanks |
If this is the case I think this might be a bug in the old process (probably out of scope for this PR).
For the new process we might actually want to start at the uppermost .tool-versions file, installing versions in order, and work our way down to the current directory. I feel like the home/root dir is typically used for system wide stuff that the user just happens to be using asdf for (ie version doesn't vary between dirs). This is definitely a nitpick. No need to implement right now.
Yep, exactly!
Interesting, I wonder if that is best. I don't think we should be doing anything until all required plugins (ie all those referenced in all .tool-version files at and above the current dir) are installed. Again, definitely out of scope for this PR but I think it may have ended up this way by accident rather than by design. Any idea why we might want to silently ignore missing plugins at this step? |
# install tools from .tool-versions | ||
# install order is the order listed in .tool-versions | ||
file_name=$(asdf_tool_versions_filename) | ||
tools_installed=$(_install_directory_tools "$search_path" "$file_name" "$plugins_installed" "$tools_installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm used to wrapping everything in quotes. Is there a reason the $()
on these two lines aren't quoted? Like this:
file_name="$(asdf_tool_versions_filename)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary. Word splitting is not done on values in assignments (see Bash Shell Parameters), i.e., quotes are not needed when assigning command substitutions ($(...)
) or variables ($var
). E.g.:
foo='a space-filled value'
bar=$foo
echo $bar
a space-filled value
foo=$(echo this is a command substitution with spaces)
echo $foo
this is a command substitution with spaces
return 0 | ||
fi | ||
|
||
tool_versions=$(strip_tool_version_comments "$search_path/$file_name" | awk '{$1=$1};1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want this $()
to be quoted right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to be honest. I noticed that most of the assignments in the installs.bash
file do not put quotes around the $()
. When I ran the linter, the linter pulled up times where I had passed a $()
to a bash function if I didn't wrap it in quotes. It would give the following warning: SC2046 (warning): Quote this to prevent word splitting.
. This makes me think that variable assignment is okay but potentially passing as arguments without wrapping in quotes is the one that isn't right. Not sure though, no expert on bash myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically if the value produced by the expression contains spaces and you want it to stay as a string you should quote - string_with_spaces="$(echo "string with spaces")"
. I think that is the case here, but I may be wrong. Generally, quoting is safer in Bash (unless you are dealing with an array, in which case it won't work).
lib/functions/installs.bash
Outdated
display_debug() { | ||
if [[ $DEBUG = "true" ]]; then | ||
printf "debug: %s\n" "$1" >&2 | ||
fi | ||
} | ||
|
||
display_none() { | ||
printf "" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to put these in utils.sh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @chrisjpalmer ! Overall I think this is a good change.
Having been away from Bash for a little while I'm a little paranoid of unquoted variables and don't remember exactly when things should be quoted or unquoted. I'll test out this PR branch on my machine today and see how it goes.
Thanks @Stratus3D for your review. Appreciate that time you put into it. I have tried to address all your comments and respond to anything that I still wasn't sure about.
Quite possibly. We can always follow this PR up with another one which hardens this behaviour.
I would be happy to chase this up in a follow up PR.
Ah I think when I said that "a version without a corresponding plugin will not contribute to the installation process" what I meant was, ignoring the sanity check that runs before installation kicks off, the main installation loop ignores unknown plugins. However there is a sanity check that runs before the main installation which lets the user know if it detects an unknown plugin. This is only in place for the |
done | ||
plugins_installed=$(printf "%s" "$plugins_installed" | tr "\n" " " | awk '{$1=$1};1') | ||
display_debug "install_local_tool_versions: plugins_installed='$plugins_installed'" | ||
tools_installed=$(install_directory_tools_recursive "$search_path" "$plugins_installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the $()
on lines 122 and 124 should be quoted.
while [ "$search_path" != "/" ]; do | ||
# install tools from files in current directory | ||
display_debug_hr | ||
tools_installed=$(install_directory_tools "$search_path" "$plugins_installed" "$tools_installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quote this one too I think.
fi | ||
|
||
# go up a directory | ||
search_path=$(dirname "$search_path") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one as well. paths can contain spaces but they should be treated like a single string.
local legacy_config | ||
legacy_config=$(get_asdf_config_value "legacy_version_file") | ||
if [ "$legacy_config" = "yes" ]; then | ||
tools_installed=$(_install_directory_tools_legacy "$search_path" "$plugins_installed" "$tools_installed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quote $()
here too I think.
for plugin_version in "${parts[@]:1}"; do | ||
# install the version | ||
display_none "$(install_tool_version "$plugin_name" "$plugin_version")" | ||
tools_installed=$(printf "%s %s" "$tools_installed" "$plugin_name" | awk '{$1=$1};1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quote $()
here too I think.
|
||
# install the version | ||
display_none "$(install_tool_version "$plugin_name" "$plugin_version")" | ||
tools_installed=$(printf "%s %s" "$tools_installed" "$plugin_name" | awk '{$1=$1};1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quote here.
|
||
# extract plugin legacy information | ||
local plugin_path | ||
plugin_path=$(get_plugin_path "$plugin_name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quote here. Path may contain whitespace.
|
||
# extract plugin legacy filenames | ||
local legacy_filenames="" | ||
legacy_filenames=$("$legacy_list_filenames_script") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For good measure quote here. I can't imagine any legacy file names would contain whitespace since these are typically filenames chosen for software developers, but it won't hurt.
|
||
display_none() { | ||
printf "" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is display_none
for?
@@ -156,6 +156,20 @@ display_error() { | |||
printf "%s\n" "$1" >&2 | |||
} | |||
|
|||
display_debug_hr() { | |||
display_debug "--------------------------------------------------------------------------------------------------------------" | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we only use this function once. Are we certain we need it?
Thanks @chrisjpalmer! just scanned through the code and left some comments. I see our build is broken here, but it isn't due to your changes. I'll see about fixing it on |
Just checking in on this as it's been a while @Stratus3D. This would be an extremely useful feature to have implemented especially for python + poetry. Seems like the remaining asks are easy cleanups that could be knocked out in 2 min |
UPDATE
Summary
This PR changes the behaviour of
asdf install
to install tools in the order they are listed in.tool-versions
.The purpose is to prevent installation failures due to dependencies between plugins. An example is the
poetry
python
plugins. When runningasdf install
for these, thepoetry
plugin will typically fail because python is not available (see here). This requires the user to manually runasdf install python xxx
first and then continue with the rest of the install.Rather than maintain the correct plugin order as part of the tool, this responsibility can be forwarded to users of the tool via
.tool-versions
file.asdf install
will respect the order listed in.tool-versions
when installing the tools.Other Information
asdf update --head should checkout the master branch
plugin_list_all should sync repo when check_duration set to 0
.tool-versions
file one by one.tool-versions
fileExample
Given the following
.tool-versions
:before
after
... old PR description
Summary
This PR allows
asdf
to install tools in the order they are listed in.tool-versions
. It works by the commandasdf install --ordered-install
to be provided.The purpose is to prevent installation failures due to dependencies between plugins. An example is the
poetry
python
plugins. When runningasdf install
for these, thepoetry
plugin will typically fail because python is not available (see here). This requires the user to manually runasdf install python xxx
first and then continue with the rest of the install.The PR takes a simple approach to resolve this problem. Rather than maintain the correct plugin order as part of the tool, which is much too difficult, allow developers to maintain the order in
.tool-versions
file. Then make theasdf
tool follow that order when--ordered-install
is provided.Other Information
I have not written any tests, nor have I done extensive testing to verify this doesn't break any old functionality.
I also have not addressed this change for multiple levels of
.tool-versions
files in higher directories.I can look into these things, if people like this change and we think its worth pursuing. Do let me know.
Our company uses
asdf
extensively for streamlining developer workflows so we are interested in improving the tool if possible.Thanks!