Skip to content
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: Support configurable ASDF_CONCURRENCY #1532

Merged
merged 5 commits into from
Apr 19, 2023

Conversation

hyperupcall
Copy link
Contributor

Summary

Fixes #1097

Other Information

Use by either setting:

  • environment variable ASDF_CURRENCY
  • config key concurrency

@hyperupcall hyperupcall requested a review from a team as a code owner April 6, 2023 11:19
@hyperupcall hyperupcall changed the title feat: Support ASDF_CONCURRENCY feat: Support configuring ASDF_CONCURRENCY Apr 6, 2023
@jthegedus
Copy link
Contributor

jthegedus commented Apr 6, 2023

Thanks for working on this 🙇 . I left some comments about handling defaults similar to the rest of the config values.

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Apr 6, 2023

Thank you for your feedback - It's good to be consistent and I'll apply the feedback later today soon

@hyperupcall hyperupcall force-pushed the add-asdf-concurrency branch from e6c1ca4 to a493b36 Compare April 17, 2023 02:18
@jthegedus
Copy link
Contributor

Ping me in a comment when this is ready for re-review

@hyperupcall
Copy link
Contributor Author

@jthegedus

@jthegedus
Copy link
Contributor

The implementation here looks good. Only a docs nitpick, though I am happy to merge and fix docs later if we don't reach a consensus on the docs nit.

@jthegedus
Copy link
Contributor

@hyperupcall If you're happy with the proposed changes I made, merge. If not, please revert and merge.

@hyperupcall
Copy link
Contributor Author

lgtm! ^.^

@hyperupcall
Copy link
Contributor Author

hyperupcall commented Apr 18, 2023

Hmm, I'm looking at the Default: unset environment variable, line again and it just seems odd to write that here. All of these variables are by default unset externally, except for I believe ASDF_DIR and ASDF_DATA_DIR on some shells. When I say externally, I mean in the user's environment, in their current shell.

The fact that the environment is unset is an implementation detail because the environment variable is set only within the asdf Bash process. So I think writing that it's unset is unhelpful at best and confusing at worst, since it's inconsistent with the other environment variables.

Is it implied that default means "Effective default"? I think so, but if the consensus is not, then we may need to either update the defaults of the other environment variables or change something else.

@jthegedus
Copy link
Contributor

jthegedus commented Apr 19, 2023

The other env vars are certainly more straightforward compared to this one. These are how they are all handled.

ASDF_CONFIG_FILE

asdf/lib/utils.bash

Lines 23 to 25 in a1e858d

asdf_config_file() {
printf '%s\n' "${ASDF_CONFIG_FILE:-$HOME/.asdfrc}"
}

ASDF_DEFAULT_TOOL_VERSIONS_FILENAME

asdf/lib/utils.bash

Lines 19 to 21 in a1e858d

asdf_tool_versions_filename() {
printf '%s\n' "${ASDF_DEFAULT_TOOL_VERSIONS_FILENAME:-.tool-versions}"
}

ASDF_DIR

asdf/lib/utils.bash

Lines 41 to 51 in a1e858d

asdf_dir() {
if [ -z "$ASDF_DIR" ]; then
local current_script_path=${BASH_SOURCE[0]}
printf '%s\n' "$(
cd -- "$(dirname "$(dirname "$current_script_path")")" || exit
printf '%s\n' "$PWD"
)"
else
printf '%s\n' "$ASDF_DIR"
fi
}

ASDF_DATA_DIR

asdf/lib/utils.bash

Lines 27 to 39 in a1e858d

asdf_data_dir() {
local data_dir
if [ -n "${ASDF_DATA_DIR}" ]; then
data_dir="${ASDF_DATA_DIR}"
elif [ -n "$HOME" ]; then
data_dir="$HOME/.asdf"
else
data_dir=$(asdf_dir)
fi
printf "%s\n" "$data_dir"
}

ASDF_CONCURRENCY - Proposed

https://github.com/hyperupcall/asdf/blob/da1f5ba0263d9656884b49fe164b0d86cc59e3d2/lib/functions/installs.bash#L27-L49

get_concurrency() {
  local asdf_concurrency=

  if [ -n "$ASDF_CONCURRENCY" ]; then
    asdf_concurrency="$ASDF_CONCURRENCY"
  else
    asdf_concurrency=$(get_asdf_config_value 'concurrency')
  fi

  if [ "$asdf_concurrency" = 'auto' ]; then
    if command -v nproc &>/dev/null; then
      asdf_concurrency=$(nproc)
    elif command -v sysctl &>/dev/null && sysctl hw.ncpu &>/dev/null; then
      asdf_concurrency=$(sysctl -n hw.ncpu)
    elif [ -f /proc/cpuinfo ]; then
      asdf_concurrency=$(grep -c processor /proc/cpuinfo)
    else
      asdf_concurrency="1"
    fi
  fi

  printf "%s\n" "$asdf_concurrency"
}

Most of these don't interact with the .asdfrc configuration file. They are usually Env Var or something else, not Env Var or 5 other code branches.

Perhaps we should change the language of all of these env vars docs. Instead of describing the value with "Default: abc" we describe the affect with "If Unset: abc".

Perhaps something like this:


Environment Variables

Setting environment variables varies depending on your system and Shell. Default locations depend upon your installation location and method (Git clone, Homebrew, AUR).

Environment variables should generally be set before sourcing asdf.sh/asdf.fish etc. For Elvish set above use asdf.

The following describe usage with a Bash Shell.

ASDF_CONFIG_FILE

Path to the .asdfrc configuration file. Can be set to any location. Must be an absolute path.

  • If Unset: $HOME/.asdfrc will be used.
  • Usage: export ASDF_CONFIG_FILE=/home/john_doe/.config/asdf/.asdfrc

ASDF_DEFAULT_TOOL_VERSIONS_FILENAME

The filename of the file storing the tool names and versions. Can be any valid filename. Typically, you should not set this value unless you want to ignore .tool-versions files.

  • If Unset: .tool-versions will be used.
  • Usage: export ASDF_DEFAULT_TOOL_VERSIONS_FILENAME=tool_versions

ASDF_DIR

The location of asdf core scripts. Can be set to any location. Must be an absolute path.

  • If Unset: the parent directory of the bin/asdf executable is used.
  • Usage: export ASDF_DIR=/home/john_doe/.config/asdf

ASDF_DATA_DIR

The location where asdf will install plugins, shims and tool versions. Can be set to any location. Must be an absolute path.

  • If Unset: $HOME/.asdf if it exists, or else the value of ASDF_DIR
  • Usage: export ASDF_DATA_DIR=/home/john_doe/.asdf

ASDF_CONCURRENCY

Number of cores to use when compiling the source code. If set, this value takes precedence over the asdf config concurrency value.

  • If Unset: the asdf config concurrency value is used.
  • Usage: export ASDF_CONCURRENCY=32

Full Configuration Example

Following a simple asdf setup with:

  • a Bash Shell
  • an installation location of $HOME/.asdf
  • installed via Git
  • NO environment variables set
  • NO custom .asdfrc file

would result in the following outcomes:

Configuration Value Calculated by
config file location $HOME/.asdfrc ASDF_CONFIG_FILE is empty, so use $HOME/.asdfrc
default tool versions filename .tool-versions ASDF_DEFAULT_TOOL_VERSIONS_FILENAME is empty, so use .tool-versions
asdf dir $HOME/.asdf ASDF_DIR is empty, so use parent dir of bin/asdf
asdf data dir $HOME/.asdf ASDF_DATA_DIR is empty so use $HOME/.asdf as $HOME exists.
concurrency auto ASDF_CONCURRENCY is empty, so rely on concurrency value from the default configuration (defaults)
legacy_version_file no from the default configuration (defaults)
use_release_candidates no from the default configuration (defaults)
always_keep_download no from the default configuration (defaults)
plugin_repository_last_check_duration 60 from the default configuration (defaults)
disable_plugin_short_name_repository no from the default configuration (defaults)

@hyperupcall thoughts?
I'll make these changes and merge in ~2hrs and any further feedback we can address in a follow up PR. Want to get this across the line ASAP.

@jthegedus
Copy link
Contributor

jthegedus commented Apr 19, 2023

Merging, any further feedback we can address in a follow up PR 🙇

Thanks @hyperupcall

@jthegedus jthegedus changed the title feat: Support configuring ASDF_CONCURRENCY feat: Support configurable ASDF_CONCURRENCY Apr 19, 2023
@jthegedus jthegedus merged commit 684f4f0 into asdf-vm:master Apr 19, 2023
@hyperupcall hyperupcall deleted the add-asdf-concurrency branch April 20, 2023 11:24
@hyperupcall
Copy link
Contributor Author

Sounds good, get this in first 👍

@Stratus3D
Copy link
Member

I've been out of the loop on this - why does asdf need to even manage concurrency? Why can't plugins just determine this on their own using existing tools outside of asdf?

@jthegedus
Copy link
Contributor

jthegedus commented Apr 21, 2023

why does asdf need to even manage concurrency

It doesn't, and probably shouldn't, but it has done so for ~7 years.

Why can't plugins just determine this on their own using existing tools outside of asdf

They can, but the fact it became part of core is an indicator that plugin authors don't want that responsibility. It's the same issue we have with people requesting we provide sort_versions, get_arch etc type of utility functions to the plugins. The authors don't want to rewrite the same code across multiple plugins.

Whether plugin author's expectations are valid is another discussion.


During the initial growth of the project, I don't think there was a clear plan or consensus on how to handle public APIs. People requesting features and raising PRs meant activity and growth of the tool, so maintainers were perhaps not as stringent as they should have been. I think the existence of the plugin repo is another example of this.

I'm happy to delete and remove code and features. But if we're making larger determinations about the plugin API and our intent to remove things, we should also carve out a clear picture of a goal-state we're aiming towards. It would then help justify to plugin authors and contributors our intentions and perhaps ease the pain of removing features.

It would be interesting to write a script to check the code of each of the plugins in the plugin repo for use of the ASDF_CONCURRENCY env var.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASDF_CONCURRENCY should be configurable
3 participants