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

asdf: install into libexec and bin.write_exec_script to opt_libexec/bin/asdf #73173

Closed
wants to merge 19 commits into from

Conversation

seivan
Copy link
Contributor

@seivan seivan commented Mar 15, 2021

Changes

  • Installing under libexec to maintain relative paths for sourcing scripts.
  • No longer installing files under lib
  • Binary in /usr/local/bin/ will point to /usr/local/opt/asdf/libexec/bin/asdf to keep the shims from breaking on updates.
  • Caveat added for new users to:
    • Optionally sourcing asdf/libexec/lib/asdf.sh if needing $ asdf shell <package> <version>
    • Export shims in$PATH since asdf/libexec/lib/asdf.sh does not do that. As opposed to asdf/libexec/asdf.sh
  • With changes suggested by @cho-m
    • Existing shims will not break, will instead having a layer of indirection. New shims will not do that.
    • Current sourcing of asdf/asdf.sh will not break as it points to asdf/libexec/asdf.sh
    • Existing users do not need to update their $PATH if they are currently sourcing asdf/asdf.sh as it points to asdf/libexec/asdf.sh

Migration

No need with migration for existing installation.
So, existing solutions such as source "$(brew --prefix asdf)/asdf.sh" will work fine

But can also add shim manually to $PATH
and optionally do source "$(brew --prefix asdf)/libexec/lib/asdf.sh" for $ asdf shell <package> <version>

Caveat

The caveat currently pending review mentions the "newer" approach, it's not necessary to follow through for current users.
Updating $PATH => Mandatory
Sourcing libexec/lib/asdf.sh" => Optional

  def caveats
    <<~EOS
      Add shims in $PATH by having the following line your ~/.zshenv or #{shell_profile}:
        export PATH="${ASDF_DATA_DIR:-$HOME/.asdf}/shims:$PATH"

      To support package version per session using asdf shell <name> <version>
      Add the following line to your #{shell_profile} file:
         . #{opt_libexec}/lib/asdf.sh
      Restart your terminal for the settings to take effect.
    EOS
  end

Relevant PR(s):

Code
asdf-vm/asdf#897
Documentation
asdf-vm/asdf#898

Motivation

  1. asdf should not create symlinks to /usr/local/lib
  2. asdf should not require sourcing a file in order to display asdf help after installing.
  3. asdf should not require sourcing a file that modifies $PATH, instead expect users themselves to define what's needed in ENV variables, if that's a custom asdf directory, or setting their shims directory under $PATH, for zsh that would fall under .zshenv and not .zshrc.

If a user can't call asdf help without getting an error after installing it without additional configuration, then I consider it a bug.
There are situations where additional setup is mandatory, like databases, but asdf is and should not be treated as such
Beyond that, the current Homebrew formula for a what essentially constitutes a collection of shell script that depend on each other is implemented wrong.
This is the most important take away from this.

Fixing:
asdf-vm/asdf#785
asdf-vm/asdf#891
asdf-vm/asdf#607
asdf-vm/asdf#394
asdf-vm/asdf#428

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Install a libexec under bin to maintain relative paths for sourcing scripts. 
This will target `bin/asdf` that will source other shell scripts, like 
`source "$(dirname "$(dirname "$0")")/lib/utils.bash"`

Probably need approval and eyes from maintainer(s)
Fix: asdf-vm/asdf#891
@BrewTestBot BrewTestBot added the bottle unneeded [DEPRECATED] Bottle does not need to be built label Mar 15, 2021
Remove dependencies unrelated to this formula. 
It doesn't fall on this formula to require those since it works without them.
No longer conflicting with `homeshick` as we're not polluting `/usr/local/lib` with commands or whatever it claimed.
Formula/asdf.rb Outdated Show resolved Hide resolved
Formula/asdf.rb Show resolved Hide resolved
Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seivan A few questions:

  • what ramifications will this change have for someone updating via brew? - will this cleanup existing location of resources, installed plugins, shims etc?
  • will users need to reinstall everything managed by their current asdf setup?

Please define the migration path before making this change.

Users who are sourcing files in their Shell configurations will presumably need to update those as you comment here asdf-vm/asdf#607 (comment) ? Won't this break people's systems?

Formula/asdf.rb Outdated Show resolved Hide resolved
asdf uses both the presence of a file OR git to check if updates are possible or not. 
No need to add a file, since git will fail on the fact that it's not a repo. 
Fix: Homebrew#73173 (comment)
@seivan
Copy link
Contributor Author

seivan commented Mar 15, 2021

@seivan A few questions:

  • what ramifications will this change have for someone updating via brew? - will this cleanup existing location of resources, installed plugins, shims etc?

Good question, @jthegedus

If you look closely I have bumped the revision here https://github.com/Homebrew/homebrew-core/pull/73173/files#diff-298ebac561811ce26c00772e4542290af157aebc91a0e05fb9270ea3e5fbbb2eR7

First of lets talk about how Homebrew does file organization for packages and updates.
We bumped the formula revision, but not the asdf version, but it should technically be the same process!

Which means that the new path will be /usr/local/Cellar/asdf/0.8.0_2/
So anything that refers to the previous path /usr/local/Cellar/asdf/0.8.0_1/ will break.
Keep in mind this is just standard procedure with Homebrew and not related to my changes.

So lets take shims for a second.

So my current shims are defined as

#!/usr/bin/env bash
# asdf-plugin: elixir 1.11.3
exec /usr/local/Cellar/asdf/0.8.0_1/libexec/bin/asdf exec "iex" "$@"

But with this update, they need to be

#!/usr/bin/env bash
# asdf-plugin: elixir 1.11.3
exec /usr/local/Cellar/asdf/0.8.0_2/libexec/bin/asdf exec "iex" "$@"

Again this is just how Homebrew does updates.

So you need to delete shims and call $ asdf reshim, like this: $ rm -rf ~/.asdf/shims; asdf reshim

So we got two choices here,

A) This pull request on asdf gets accepted where $ asdf reshim always rewrites shims from scratch, which I think is a better choice, but I don't understand why it didn't do to begin with, could be some reason for it. This change combined with a post_install hook that calls reshim

  def post_install
  	system bin/"asdf", "reshim"
  end

The benefits for this approach is that we always call reshim on asdf updates and those might have new changes necessary, which is a good idea.

B) The alternative is that we remove the cellar version /usr/local/Cellar/asdf/0.8.0_1/libexec/bin/asdf by exec:ing against opt_prefix instead.

That means the shims would be defined as

#!/usr/bin/env bash
# asdf-plugin: elixir 1.11.3
exec /usr/local/opt/asdf/libexec/bin/asdf exec "iex" "$@"

I am not sure what approach is better. If demanding reshims on asdf updates is considered bad, then the second option is a good choice. But I personally think that a new version of asdfs warrants a reshim just to keep things maintained.
Also I do think that having asdf namespaced under a version is good hygiene, but I am not 100% on this.

But for now I've decide to use opt_prefix/"libexec, or more specifically opt_libexec. This means you don't have to worry about shims when updating asdf.

  • will users need to reinstall everything managed by their current asdf setup?

Simple answer: no, nothing needs to be reinstalled.

The migration itself is simple, but for a verbose explanation of my testing, these are the steps I did for finding out what needs to be changed.

  1. Install "origin" asdf via brew => $ brew install asdf.
  2. Adding plugins from asdf-vm/asdf-elixir, seivan/asdf-erlang and seivan/asdf-ruby.
  3. Installing globals from plugins defined in step 2.
  4. Uninstalling old asdf => brew uninstall asdf; brew cleanup; brew autoremove.
  5. By now, I am getting an error since I am sourcing a file that doesn't exist with source "$(brew --prefix asdf)/asdf.sh".
  6. Removing source "$(brew --prefix asdf)/asdf.sh" from my profile (.zshrc).
  7. Installing my tap $brew install seivan/custom/asdf which is the formula you see here.
  8. Adding the executables to my path: export PATH="$HOME/.asdf/shims:$PATH" in .zshenv.
  9. Run reshim => rm -rf ~/.asdf/shims; asdf reshim.

Please define the migration path before making this change.

Migration

  1. Remove source "$(brew --prefix asdf)/asdf.sh" from your shell config e.g .zshrc

  2. Add export PATH="$HOME/.asdf/shims:$PATH" to your env profile, e.g. .zshenv where it belongs(!).

  3. Reshim by running: rm -rf ~/.asdf/shims; asdf reshim Don't need unless you think keeping shims tied to specific versions of asdf is important enough to warrant this, but in that case we need this change

To expand on step 2.

So normally if you'd want to run Ruby, you'd do ~/.asdf/shims/ruby, but obviously you don't want to define the whole path each time, so by adding the shims to $PATH, you can just get it by $ruby.
It sounds like I am explaining $PATH to you, but what I am really trying (poorly) to point out is that, this doesn't fall on asdf.

These changes make asdf itself work out of the box even if you don't setup the shims in $PATH. Which is different from current formula, where installing asdf and calling asdf simply just breaks with an error, which it should never do, if users can't even call asdf help after installing it, then I'd consider it a bug.
This is the most important take away from this.

If your shims are in the default directory ($HOME/.asdf/shims) then you need to add that to your $PATH, as you would with any accessible executables.

However, asdf lets you designate a custom directory, so it's basically just more of the same inside your env profile, e.g .zshenv

export ASDF_DATA_DIR="$HOME/.config/versioned_tools/asdf"
export PATH="$ASDF_DATA_DIR/shims:$PATH"

This is actual configuration that I can get behind and mimics other shell configurations most people got.

Users who are sourcing files in their Shell configurations will presumably need to update those as you comment here asdf-vm/asdf#607 (comment) ? Won't this break people's systems?

Correct, but that's because current installation of asdf via Homebrew is in my opinion faulty and so far the suggestions have been work arounds by requiring people to source a file when it should just read ENV variables defined at your shell environment, e.g .zshenv and work out of the box. Not to mention that it clutters file under /usr/local/lib and a binary that doesn't work to even call help on its own inside /usr/local/bin unless you source a second file.

@seivan
Copy link
Contributor Author

seivan commented Mar 15, 2021

@seivan
Copy link
Contributor Author

seivan commented Mar 15, 2021

@jthegedus I've added support for making the migration a little bit less painful here - this would work as post install on Homebrew, but I could use some context on why it was just comparing meta-data before rewriting shims.

@carlocab carlocab changed the title Update asdf.rb asdf: install into libexec instead Mar 15, 2021
@carlocab
Copy link
Member

I've renamed the title of your PR to conform to homebrew/core style [1], but feel free to edit it if it doesn't reflect the changes you're proposing.

[1] We put the name of the formula first in commit-message headings. Refer to the documentation for more details.

@seivan
Copy link
Contributor Author

seivan commented Mar 15, 2021

I've renamed the title of your PR to conform to homebrew/core style [1], but feel free to edit it if it doesn't reflect the changes you're proposing.

This is better, sorry I should have done better than the default one. Thanks!

Switch to `opt_libexec` which translates to `opt_prefix/"libexec`, this means that there will be no migration steps for shims. 
However till adding a `$ asdf reshim` `post_install` hook incase there is an update that needs to be done on the shims when `asdf` itself updates to a new version or formula revision. There are changes pending on  [`asdf-vm`](asdf-vm/asdf#893) that would allow regenerating the shims in case they need to be.
@seivan
Copy link
Contributor Author

seivan commented Mar 15, 2021

Switch to opt_libexec which translates to opt_prefix/"libexec, this means that there will be no migration steps for shims.
However till adding a $ asdf reshim post_install hook incase there is an update that needs to be done on the shims when asdf itself updates to a new version or formula revision. There are changes pending on asdf-vm that would allow regenerating the shims in case they need to be.

Formula/asdf.rb Outdated Show resolved Hide resolved
Co-authored-by: Carlo Cabrera <[email protected]>
@seivan seivan changed the title asdf: install into libexec instead asdf: install into opt_libexec instead Mar 15, 2021
@seivan seivan changed the title asdf: install into opt_libexec instead asdf: install into libexec and bin.write_exec_script to opt_libexec/bin/asdf instead Mar 15, 2021
Fix lint issues.
Adding adding "asdf_updates_disabled" file to libexec, to disallow updates since it might be a git repo on m1/arm. 
Fix: https://github.com/Homebrew/homebrew-core/pull/73173/files/b517eb00222262961583460e96f3379bb01c8d57#r594787883
Add `git` and `coreutils` as dependencies because `asdf` needs to be written as cross platform this will help maintainers not to worry about platforms.
Fix CI lint issues.
More linting crap.
@cho-m
Copy link
Member

cho-m commented Mar 17, 2021

If I were to guess it's changing the version of a specific language for the current session.

Or if I take the docs literally: ...
It's just setting the environment variable?

Both should be correct. shell is one of the available scopes (shell | local | global) and can be used for quickly changing language version in a single shell session.

The implementation uses environment variables to restrict change to lifespan of that single session. The longer living scopes local | global are done by writing into .tool-versions files, but writing to file to deal with single session would require storing extra metadata and increase complexity.

I believe this was adopted from pyenv / rbenv usage of the same 3 scopes.

I mean if all $asdf shell does is evaling $ asdf export-shell-version shell <language> <version> why is it not just another command?

Edit: Right, so the parents scope won't inherit the environment variable.

Yeah, since bin/asdf will run inside a Bash subshell, it won't be able to manipulate the current shell's environment variables. The function wrapper in lib/asdf.{sh,fish} gets around that by running eval(posix) or source(fish) on asdf's printed exports.


So, to get feature parity, the original:

source "$(brew --prefix asdf)/asdf.sh"

will need to become either:

  •  export PATH="${ASDF_DATA_DIR:-$HOME/.asdf}/shims:$PATH"
     source "$(brew --prefix asdf)/libexec/lib/asdf.sh"
  •  source "$(brew --prefix asdf)/libexec/asdf.sh"

EDIT: Not too sure of Homebrew's stance on this, but another alternative for backwards-compatible migration would be to consider additional wrapper scripts.

It really depends on if a cleaner install is better vs. lower user impact (or preventing future asdf/brew GitHub issues due to the fact that most people would never see the documentation in this PR and just find asdf not working after brew upgrade).

Maybe something like:

(prefix/"asdf.sh").write "source #{opt_libexec}/asdf.sh\n"
(prefix/"asdf.fish").write "source #{opt_libexec}/asdf.fish\n"

@seivan
Copy link
Contributor Author

seivan commented Mar 18, 2021

@cho-m

The implementation uses environment variables to restrict change to lifespan of that single session. The longer living scopes local | global are done by writing into .tool-versions files, but writing to file to deal with single session would require storing extra metadata and increase complexity.

Ah, thanks for explaining that!

will need to become either:

  •  export PATH="${ASDF_DATA_DIR:-$HOME/.asdf}/shims:$PATH"
     source "$(brew --prefix asdf)/libexec/lib/asdf.sh"

Yeah, I actually think this is the best approach. I personally will only do

 export PATH="$HOME/.asdf/shims:$PATH"

in my .zshenv and that will suffice for me.

Because I don't need shell but I can totally see others wanting it.

But this is something you can explain inside $ asdf help it doesn't need a caveat.
Right now the actual fix isn't how to shoe-horn that in, but to make the installation not to throw an error when you call $ asdf or $ asdf help.

It really depends on if a cleaner install is better vs. lower user impact (or preventing future asdf/brew GitHub issues due to the fact that most people would never see the documentation in this PR and just find asdf not working after brew upgrade).

Maybe something like:

(prefix/"asdf.sh").write "source #{opt_libexec}/asdf.sh\n"
(prefix/"asdf.fish").write "source #{opt_libexec}/asdf.fish\n"

No, please don't do this. Don't source code automatically for people, it's already bad that enough that they modify my $PATH I also don't want anything else to have potential to slow down my shell without me opting in.

I personally don't need $ asdf shell but if I ever do, I can just export whatever is outputted by
$ asdf export-shell-version shell <language> <version>.
Though I can see others wanting it, it's better to just document it on both the website and put it in $ asdf shell directly instead of just throwing an error.

  asdf_cmd() {
  local ASDF_CMD_FILE args_offset

  if [ "shell" == "$1" ]; then
    echo "Shell integration is not enabled. Please ensure you source asdf in your shell setup." >&2
    exit 1
  fi

Have that explain how to set up $ asdf shell and have $asdf help explain how to both add shims to $PATH and add shell support as an optional step.

@carlocab
Copy link
Member

Maybe something like:

(prefix/"asdf.sh").write "source #{opt_libexec}/asdf.sh\n"
(prefix/"asdf.fish").write "source #{opt_libexec}/asdf.fish\n"

No, please don't do this. Don't source code automatically for people, it's already bad that enough that they modify my $PATH I also don't want anything else to have potential to slow down my shell without me opting in.

This doesn't source code automatically for anyone. One still needs to source ${HOMEBREW_PREFIX}/opt/asdf/asdf.sh for this to do anything.

@seivan
Copy link
Contributor Author

seivan commented Mar 18, 2021

Maybe something like:

(prefix/"asdf.sh").write "source #{opt_libexec}/asdf.sh\n"
(prefix/"asdf.fish").write "source #{opt_libexec}/asdf.fish\n"

No, please don't do this. Don't source code automatically for people, it's already bad that enough that they modify my $PATH I also don't want anything else to have potential to slow down my shell without me opting in.

This doesn't source code automatically for anyone. One still needs to source ${HOMEBREW_PREFIX}/opt/asdf/asdf.sh for this to do anything.

Ah you are correct. I'll leave that up to you because I don't really care about $ asdf shell. I just want the binary installed in /usr/local/bin and want to point to opt_libexec/"bin/asdf" so the damn thing just works on install.
At the same time I don't the formula to install any other files in any other /usr/local/** directory.

Everything else is secondary, you guys come to an agreement on what you want and make a suggestion.

@cho-m
Copy link
Member

cho-m commented Mar 18, 2021

@seivan @carlocab I'll add a review comment so that further discussion can be contained there.

I did apply the PR Patch File along with these extra writes, and I think the result was a no-impact upgrade where all asdf commands, installs, and shims kept working.

I also did install a version with only the PR changes. In my case, I usually contain all source/eval in if [ ] statements since I share my dotfiles across machines. For someone with a similar zsh config as mine but not aware of the changes, the first time they would experience an issue would be when they realize their shims are missing.

If we decide not to do this, then part of migration would probably end up passed to @jthegedus and others on ASDF side since some Homebrew users will end up going to that repo first. ASDF will need to have corresponding documentation ready for when the PR gets merged to avoid unnecessary issues opened.

EDIT: I'll note that outside of these last migration/documentation concerns, I think everything else in PR looks good to go based on my perusal and some basic local install tests.

Formula/asdf.rb Show resolved Hide resolved
end

def post_install
system bin/"asdf", "reshim"
end

Copy link
Member

@cho-m cho-m Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another optional change mainly for documentation/migration.
May require further editing depending on recommendations from Homebrew maintainers on best way to present caveats and depending on decision for the previous optional change.

Caveat text based on autojump.rb and chruby.rb

Suggested change
def caveats
<<~EOS
Add the following line to your ~/.bash_profile or ~/.zshrc file:
source #{opt_libexec}/asdf.sh
If you use Fish shell then add the following line to your ~/.config/fish/config.fish:
source #{opt_libexec}/asdf.fish
Restart your terminal for the settings to take effect.
EOS
end

For users who just want all of asdf working after install, I think this is the best option.
Since asdf documentation recommends sourcing this for all users (Linux/macOS and git/homebrew), I think this should be default recommendation.

For users who want to do the minimal changes like only exporting shim paths, I think this should be left up to the users to understand how asdf.sh works and apply the correct exports themselves. This avoids asdf issues created due to doing something not documented.

Copy link
Contributor Author

@seivan seivan Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m
Again, wouldn't that modify your $PATH and also add the asdf binary again which should already be there?

In my opinion both asdf.fish and asdf.sh should be ignored by Homebrew users and they should go straight to the actual files

(using . instead of source)

        source #{opt_libexec}/lib/asdf.sh

That way $ asdf shell works and no one messes with your $PATH

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seivan
The goal is low-impact migration and low-impact maintenance.

In order to get the same behavior as before, Homebrew users would have to do at least an export (shim path) + source (shell function). Neither of these steps are currently documented by ASDF.

There is also the potential impact of missing export ASDF_DIR. Some plugins like asdf-nodejs directly use this variable. If missing this causes any breakage, then Homebrew users would have to now do 2 export + 1 source for feature parity.

Additionally, if ASDF does a change to their shell scripts, Homebrew users should not start raising ASDF issues due to following a set of instructions that were not directly recommended by ASDF. If a user wants to clean up their environment variables / PATH, they should do so with full understanding that this is not directly supported yet and any problems they encounter may be due to their own changes.

I personally think it is better to perform a migration in small stages:

  1. Update Homebrew formula + show caveat without any noticeable change to existing users
  2. Update ASDF documentation to show new location to source.
  3. Consider updating ASDF shell scripts to avoid adding $ASDF_DIR/bin to path for Homebrew users
  4. Consider adding new documentation/support for ASDF feature specific enablement, i.e.
    1. describe manually add ${ASDF_DATA_DIR:-$HOME/.asdf}/shims to PATH for base features
    2. sourcing lib/asdf.sh for shell feature.
  5. After some time and on a new ASDF semantic version, Homebrew can consider dropping support for previous (prefix/"asdf.sh"). Since we are really only doing a patch update (0.8.0_1 -> 0.8.0_2), it is best to have no noticeable impact.

Copy link
Contributor Author

@seivan seivan Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m

The goal is low-impact migration and low-impact maintenance.

I am with you in trying to make as little manual intervention as possible for migrating, but not at the cost of fixing the issue now or messing things up further to prolong the fix.

As I've stated earlier, as long as $ asdf and $ asdf help is broken on install everything else is moot.

The current setup is already broken and requires manual intervention.
Current users has most likely "mitigated" that and will require manual intervention on upgrade, there is no way around these two things.
It's better to fix it now than to prolong it and your steps are flawed, I'll go through each manually.

There is also the potential impact of missing export ASDF_DIR. Some plugins like asdf-nodejs directly use this variable. If missing this causes any breakage, then Homebrew users would have to now do 2 export + 1 source for feature parity.

Incorrect on two parts.
If someone has customized the ASDF_DATA_DIR they have also exported that variable which asdf_data_dir() function reads and it will work as intended.
If they have not, it will default to $HOME/.asdf and it will work as intended.

That nodejs plugin is confusing $ASD_DIR with $ASDF_DATA_DIR.

I have not, and it worked as intended.

Additionally, if ASDF does a change to their shell scripts, Homebrew users should not start raising ASDF issues due to following a set of instructions that were not directly recommended by ASDF.

Which instructions are you referring to?

If a user wants to clean up their environment variables / PATH, they should do so with full understanding that this is not directly supported yet and any problems they encounter may be due to their own changes.

What changes are these?

In general, I agree with you previous statement about the caveat, you just got the paths wrong

If they need asdf shell then explain to them that they should source

        . #{opt_libexec}/lib/asdf.sh

and to add executables to their $PATH the need to set this in their .profile or .zshenv

export PATH="${ASDF_DATA_DIR:-$HOME/.asdf}/shims"

That should be the caveat, that's a complete setup for bash/zsh, and something different for Fish I'd presume.

I personally think it is better to perform a migration in small stages:

Hard disagree in this case, as the setup is broken. Any other case, I'd agree with you.

  1. Update Homebrew formula + show caveat without any noticeable change to existing users
  2. Update ASDF documentation to show new location to source.

Depends on what update, and if the caveat refers to something that will be inadequate in the future.
Then you're just prolonging it

  1. Consider updating ASDF shell scripts to avoid adding $ASDF_DIR/bin to path for Homebrew users
    Consider adding new documentation/support for ASDF feature specific enablement, i.e.
    describe manually add ${ASDF_DATA_DIR:-$HOME/.asdf}/shims to PATH for base features
    sourcing lib/asdf.sh for shell feature.

Yeah, again, hard disagree.

After some time and on a new ASDF semantic version, Homebrew can consider dropping support for previous (prefix/"asdf.sh"). Since we are really only doing a patch update (0.8.0_1 -> 0.8.0_2), it is best to have no noticeable impact.

You should do this now, because you're just prolonging the issues and the mess continues.

Copy link
Contributor Author

@seivan seivan Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m

If it was unclear:

I think your code suggestions are solid for making it work for existing users migrating - assuming you have tested an existing setup with these added later.

   (prefix/"asdf.sh").write "source #{opt_libexec}/asdf.sh\n"
   (prefix/"asdf.fish").write "source #{opt_libexec}/asdf.fish\n"

I just disagree with the caveats, that's all. They should not explain the "old" approach.

Correct me if I am wrong, but with your changes suggested:

  1. A reshim is not necessary
  2. It will work out of the box for current users that have used the suggested steps from asdf docs.

Are both points correct?

That being said, I am surprised that existing shims would work.

My shims are written as

exec /usr/local/opt/asdf/libexec/bin/asdf exec "iex" "$@"

I don't think adding

   (prefix/"asdf.sh").write "source #{opt_libexec}/asdf.sh\n"

Will fix that, it just makes existing settings that source that file to still work, which is fine on its own, but doesn't serve a purpose if a user has to reshim. If they're doing that manual intervention, might as will do a full on migration.

Again, I could be wrong, if you could confirm that with those changes added your shim paths to old executable would still work, then it's game.

Copy link
Contributor Author

@seivan seivan Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m

For users who want to do the minimal changes like only exporting shim paths, I think this should be left up to the users to understand how asdf.sh works and apply the correct exports themselves. This avoids asdf issues created due to doing something not documented.

Maybe, however I don't agree but then again these are more opinions than facts. Let me make my case, and if you still dislike it, I'll revert.

First off, I don't want stuff added to my $PATH, I know macOS does this, but that's the OS, it gets a pass.

I think it's better for asdf has this in their documentation as this is where you're adding all the compilers, interpreters and other binaries, it's more explicit in what's going on.
For this reason, I think exporting shims in path should be very explicit.

Then there is the issue with a specific feature of asdf that requires sourcing. I see this as a separate thing and not related to exporting shims to $PATH

What I'm more indifferent about is if the caveat should be explicit about why you're sourcing a file.

I've added this

To support package version per session using asdf shell <name> <version>
       Add the following line to your #{shell_profile} file:
         . #{opt_libexec}/lib/asdf.sh
       If you use Fish shell then add the following line to your ~/.config/fish/config.fish:
         source #{opt_libexec}/lib/asdf.fish
       Restart your terminal for the settings to take effect.

So the choices for caveats as I see is

  1. Source the script that modifies path and adds shell function (Your suggestion)
  2. Source the script that adds shell function, keep $PATH explicit (Mix of both ours)
  3. Keep $PATH explicit, mention optional sourcing of shell function (Currently in place)

@cho-m So, how do you feel about option 3?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this question should be redirected to Homebrew maintainers (who may have explicit rules on what is allowed in caveats) and ASDF maintainers (who may have opinions on recommended installation steps).

I am neither, so I want to avoid suggesting something that doesn't line up with both projects' ideology.

Hopefully one of the Homebrew members already subscribed to this PR can respond.
They should know better on how the general user-base deals with caveat info.

I see you also have discussion on ASDF asdf-vm/asdf#891 so maybe see if there are any opinions are on that side. The decision here may require changes for ASDF's own documentation.

Copy link
Contributor Author

@seivan seivan Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m Fair enough, both @carlocab and @jthegedus wanna chime in here?

Edit: Added suggestions on how to change docs in asdf-vm/asdf#898

seivan and others added 4 commits March 19, 2021 02:03
This should technically make it work for existing users who are migrating. Per Homebrew#73173 (review)

Co-authored-by: Michael Cho <[email protected]>
Add caveat for migrating.
Add caveat for new settings.
Fix trailing whitespace for CI.
Formula/asdf.rb Outdated
Comment on lines 21 to 22
(prefix/"asdf.sh").write ". #{opt_libexec}/asdf.sh\n"
(prefix/"asdf.fish").write ". #{opt_libexec}/asdf.fish\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m So I tested, and these two lines will make sure that currently documented (on asdf website) "caveat" works for sourcing.

But I don't see how that prevents the need for a rm -rf ~/.asdf/shims; asdf reshim - can you confirm that old shims will point to the correct binary?

My shims are defined as

 cat .asdf/shims/iex    
#!/usr/bin/env bash
# asdf-plugin: elixir 1.11.3
exec /usr/local/opt/asdf/libexec/bin/asdf exec "iex" "$@"

The new executable asdf will no longer be versioned and instead always be the active one. I have opinions on this, but for now this will be the best approach until $ asdf reshim gets some fixes to it.

But /usr/local/opt/asdf/libexec/bin/asd is not the same as the old path was. Which makes me think that the purposes of these two lines are negated since a migration step is necessary, at least for removing and reshimming.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ASDF's documented installation steps with currently available Formula, my shims appear to be using $HOMEBREW_PREFIX/opt/asdf/bin/asdf. Pretty much multiple rounds of redirection. Forcing a reshim can help remove one exec, but it at least works as is.

I'm on M1 mac so my exact path is different: /opt/homebrew/opt/asdf/bin/asdf.
Also, in my example, I'm using Unix/Linux-style XDG paths.

After installing patch:

brew info asdf | grep Cellar
/opt/homebrew/Cellar/asdf/0.8.0_2 (119 files, 244.0KB) *asdf current python
python          3.9.2 system    /Users/cho-m/.config/asdf/tool-versionspython --version
Python 3.9.2pip --version
pip 21.0.1 from /Users/cho-m/.local/share/asdf/installs/python/3.9.2/lib/python3.9/site-packages/pip (python 3.9)python
Python 3.9.2 (default, Feb 21 2021, 22:26:49)
[Clang 12.0.0 (clang-1200.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import toml
>>> toml.__file__
'/Users/cho-m/.local/share/asdf/installs/python/3.9.2/lib/python3.9/site-packages/toml/__init__.py'asdf current ruby
ruby            3.0.0           /Users/cho-m/.config/asdf/tool-versionsruby --version
ruby 3.0.0p0 (2020-12-25 revision 95aff21468) [arm64-darwin20]head -n3 "$ASDF_DATA_DIR/shims/ruby"
#!/usr/bin/env bash
# asdf-plugin: ruby 3.0.0
exec /opt/homebrew/opt/asdf/bin/asdf exec "ruby" "$@"

Copy link
Contributor Author

@seivan seivan Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m

Oh my God, this is amazing news.

@jthegedus This should be of interest to you.

Just to see if I understood it...

Current shims are linked to opt/asdf/bin/asdf directly!
My PR change adds a new opt/asdf/bin/asdf that points to opt/asdf/libexec/bin/asdf via

    bin.write_exec_script (opt_libexec/"bin/asdf")

So new shims will get less indirection, but older shims still work just fine.

I didn't actually expect that to migrate so well, this is amazing news.
This combined with your added changes to create the files current users are currently sourcing

    (prefix/"asdf.sh").write ". #{opt_libexec}/asdf.sh\n"

Makes this a no change migration!
Correct?

All that's left is discussing the caveat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is what I see on my machine.
Should be a seamless migration for existing users.

Formula/asdf.rb Outdated
touch libexec/"asdf_updates_disabled"
bin.write_exec_script (opt_libexec/"bin/asdf")
(prefix/"asdf.sh").write ". #{opt_libexec}/asdf.sh\n"
(prefix/"asdf.fish").write ". #{opt_libexec}/asdf.fish\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fish is not a POSIX shell and has deprecated '.' support

. (a single period) is an alias for the source command. The use of . is deprecated in favour of source, and . will be removed in a future version of fish.

Copy link
Contributor Author

@seivan seivan Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m Thanks!

Edit, do you know how exports look like for fish?

set -l asdf_user_shims (
  if test -n "$ASDF_DATA_DIR"
    echo $ASDF_DATA_DIR/shims
  else
    echo $HOME/.asdf/shims
  end
)

Copy link
Member

@cho-m cho-m Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exports in Fish shell are done through the set -x command (though may need -g if not already global variable).

I am not entirely familiar with Fish's idiosyncrasies since the spec is still in active development compared to POSIX.

I think PATH is a special already-global variable.
Looking at Fish tutorial, prepending 2 directories is:

set PATH /usr/local/bin /usr/sbin $PATH

The part you quoted is due to Fish shell not supporting POSIX parameter expansion.
Thus, rather than ${ASDF_DATA_DIR:-$HOME/.asdf}, they need a conditional check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m

set PATH (                                                                          
  if test -n "$ASDF_DATA_DIR"                                                       
    echo $ASDF_DATA_DIR/shims                                                       
  else                                                                              
    echo $HOME/.asdf/shims                                                          
  end                                                                               
) $PATH  

Should be good enough for the caveat?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should work.

A side question is if the caveat is becoming too long. I don't know if there is a style recommendation.
Try to get a Homebrew maintainer to review everything again and decide the direction of what the caveat should say.
Hopefully should be last review to finalize PR merge.

Copy link
Contributor Author

@seivan seivan Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cho-m Earlier suggestions I had was to put all this somewhere as a command, like $ asdf setup and have that as a post_install hook, but then again not sure if that also breaks Homebrew rules, @carlocab

I suppose the reason why this wasn't possible now was because $ asdf help would just error out, but with this fix, it works out of the box regardless of sourcing or updating path.

Fish is not posix.
Should be support for this, since I've seen $PATH updates for various python packages and chruby.
Formula/asdf.rb Outdated
Comment on lines 29 to 41
def caveats
<<~EOS
Add shims in $PATH by having the following line your ~/.zshenv or #{shell_profile}:
export PATH="${ASDF_DATA_DIR:-$HOME/.asdf}/shims:$PATH"

To support package version per session using asdf shell <name> <version>
Add the following line to your #{shell_profile} file:
. #{opt_libexec}/lib/asdf.sh
If you use Fish shell then add the following line to your ~/.config/fish/config.fish:
source #{opt_libexec}/lib/asdf.fish
Restart your terminal for the settings to take effect.
EOS
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlocab
What is your take on the current caveats.

@cho-m Showed that chruby and autojump had support for sourcing files.
I also saw openssl explaining how to add to $PATH.

Fix indentation, using spaces.
seivan added a commit to seivan/asdf that referenced this pull request Mar 19, 2021
@seivan seivan changed the title asdf: install into libexec and bin.write_exec_script to opt_libexec/bin/asdf instead asdf: install into libexec and bin.write_exec_script to opt_libexec/bin/asdf Mar 19, 2021
Taking into account that macOS `path_helper` might trip some people up.
@seivan seivan closed this Apr 2, 2021
@seivan seivan deleted the patch-1 branch April 2, 2021 03:37
@jthegedus
Copy link
Contributor

jthegedus commented Apr 3, 2021

What's the impetus for closing this and accompanying PRs?

@seivan
Copy link
Contributor Author

seivan commented Apr 3, 2021

Inactive?

@jthegedus
Copy link
Contributor

jthegedus commented Apr 3, 2021

asdf is maintained by volunteers with limited time to put towards it. While slow to respond, we will once we understand the changes being proposed and if they align to our goals for the tool.

Speaking for myself, I've had very little time to put towards asdf these past couple weeks. The time I have had the past month has been spent reviewing (some of) these changes in an attempt to understand them. Please don't close them if they are still useful changes, otherwise the limited time I have had to put towards open source (specifically these issues) is then wasted.

@seivan
Copy link
Contributor Author

seivan commented Apr 3, 2021

Right, do you want me to open it again? I assumed lack of interest.

@jthegedus
Copy link
Contributor

I am very interested in improving asdf usage for our Homebrew users. If you still think this is the best solution, please do re-open.

Though I am not going to be free to review anything in detail for at least another week. Perhaps @Stratus3D may have the time between now and then?

@github-actions github-actions bot added the outdated PR was locked due to age label May 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bottle unneeded [DEPRECATED] Bottle does not need to be built outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants