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.

Sign up for GitHub

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
Closed

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

Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5014a0e
Update asdf.rb
seivan Mar 15, 2021
4b6418a
Update asdf.rb
seivan Mar 15, 2021
b517eb0
Update asdf.rb
seivan Mar 15, 2021
731290a
Update asdf.rb
seivan Mar 15, 2021
5b99c71
Update asdf.rb
seivan Mar 15, 2021
bc44890
Update Formula/asdf.rb
seivan Mar 15, 2021
9435946
Update asdf.rb
seivan Mar 15, 2021
aae56ab
Update asdf.rb
seivan Mar 16, 2021
6b9f33b
Update asdf.rb
seivan Mar 16, 2021
7ec5a42
Update asdf.rb
seivan Mar 16, 2021
b67ca06
Update asdf.rb
seivan Mar 16, 2021
9290bfb
Add migration code for existing users.
seivan Mar 19, 2021
6baf058
Update asdf.rb
seivan Mar 19, 2021
0d8ddaf
Create asdf.rb
seivan Mar 19, 2021
51e2e38
Update asdf.rb
seivan Mar 19, 2021
10aca50
Update asdf.rb
seivan Mar 19, 2021
198430f
Update asdf.rb
seivan Mar 19, 2021
c6dbcad
Update asdf.rb
seivan Mar 19, 2021
c243d9f
Update asdf.rb
seivan Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions Formula/asdf.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,30 +4,40 @@ class Asdf < Formula
url "https://github.com/asdf-vm/asdf/archive/v0.8.0.tar.gz"
sha256 "9b667ca135c194f38d823c62cc0dc3dbe00d7a9f60caa0c06ecb3047944eadfa"
license "MIT"
revision 1
revision 2
head "https://github.com/asdf-vm/asdf.git"

bottle :unneeded

depends_on "autoconf"
depends_on "automake"
depends_on "coreutils"
depends_on "libtool"
depends_on "libyaml"
depends_on "[email protected]"
depends_on "readline"
depends_on "unixodbc"

conflicts_with "homeshick",
because: "asdf and homeshick both install files in lib/commands"
seivan marked this conversation as resolved.
Show resolved Hide resolved
depends_on "git"
seivan marked this conversation as resolved.
Show resolved Hide resolved

def install
bash_completion.install "completions/asdf.bash"
fish_completion.install "completions/asdf.fish"
zsh_completion.install "completions/_asdf"
libexec.install "bin/private"
prefix.install Dir["*"]
touch prefix/"asdf_updates_disabled"
libexec.install Dir["*"]
touch libexec/"asdf_updates_disabled"
bin.write_exec_script (opt_libexec/"bin/asdf")
seivan marked this conversation as resolved.
Show resolved Hide resolved
(prefix/"asdf.sh").write ". #{opt_libexec}/asdf.sh\n"
(prefix/"asdf.fish").write "source #{opt_libexec}/asdf.fish\n"
end

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

Copy link
Member

@cho-m cho-m Mar 18, 2021 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

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.


test do
Expand Down