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

jenv: install fish function symlinks #76864

Merged
merged 4 commits into from
Jun 2, 2021

Conversation

davidxia
Copy link
Contributor

@davidxia davidxia commented May 8, 2021

The caveat isn't compatible with fish shell.
This PR changes the caveat for fish shell to show the
commands specified in the upstream's README here [1].

This PR does not change the caveat for other shells.

fixes jenv/jenv#315

I have tested this PR and get the output below.

With bash set as default shell (chsh -s $(which bash)):

brew info jenv

jenv: stable 0.5.4, HEAD
Manage your Java environment
https://www.jenv.be/
/opt/homebrew/Cellar/jenv/0.5.4 (82 files, 72.3KB) *
  Built from source on 2021-05-07 at 17:19:07
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/jenv.rb
License: MIT
==> Options
--HEAD
	Install HEAD version
==> Caveats
To activate jenv, add the following to your /Users/dxia/.bash_profile:

  export PATH="$HOME/.jenv/bin:$PATH"
  eval "$(jenv init -)"
==> Analytics
install: 9,672 (30 days), 30,663 (90 days), 129,619 (365 days)
install-on-request: 9,670 (30 days), 30,635 (90 days), 128,558 (365 days)
build-error: 0 (30 days)

With fish set as default shell:

brew info jenv

jenv: stable 0.5.4, HEAD
Manage your Java environment
https://www.jenv.be/
/opt/homebrew/Cellar/jenv/0.5.4 (82 files, 72.3KB) *
  Built from source on 2021-05-07 at 17:19:07
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/jenv.rb
License: MIT
==> Options
--HEAD
	Install HEAD version
==> Caveats
To activate jenv, run the following commands:

  echo 'set -g fish_user_paths "/opt/homebrew/opt/jenv/bin" $fish_user_paths' >> ~/.config/fish/config.fish
  echo 'status --is-interactive; and source (jenv init -|psub)' >> ~/.config/fish/config.fish
  cp /opt/homebrew/opt/jenv/libexec/fish/jenv.fish ~/.config/fish/functions/jenv.fish
  cp /opt/homebrew/opt/jenv/libexec/fish/export.fish ~/.config/fish/functions/export.fish
==> Analytics
install: 9,672 (30 days), 30,663 (90 days), 129,619 (365 days)
install-on-request: 9,670 (30 days), 30,635 (90 days), 128,558 (365 days)
build-error: 0 (30 days)

1: https://github.com/jenv/jenv/blob/9bbc5ebfe6f38252a1a0f28897c7ae52e6a483a0/README.md

  • 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>)?

@BrewTestBot BrewTestBot added the bottle unneeded [DEPRECATED] Bottle does not need to be built label May 8, 2021
@davidxia davidxia force-pushed the jenv-caveat-fish branch 3 times, most recently from e4ad91e to 855afae Compare May 8, 2021 18:34
Formula/jenv.rb Outdated Show resolved Hide resolved
@carlocab carlocab added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label May 8, 2021
Formula/jenv.rb Outdated Show resolved Hide resolved
Formula/jenv.rb Outdated Show resolved Hide resolved
Formula/jenv.rb Outdated Show resolved Hide resolved
@davidxia davidxia force-pushed the jenv-caveat-fish branch 2 times, most recently from e7c5f61 to 8821e82 Compare May 8, 2021 19:13
davidxia added a commit to davidxia/jenv that referenced this pull request May 8, 2021
Formula/jenv.rb Outdated Show resolved Hide resolved
The caveat isn't compatible with fish shell.
This PR changes the caveat for fish shell to show the
commands specified in the upstream's README here \[1\].

This PR does not change the caveat for other shells.

fixes jenv/jenv#315

I have tested this PR and get the output below.

With bash set as default shell (`chsh -s $(which bash)`):

```
brew info jenv

jenv: stable 0.5.4, HEAD
Manage your Java environment
https://www.jenv.be/
/opt/homebrew/Cellar/jenv/0.5.4 (82 files, 72.3KB) *
  Built from source on 2021-05-07 at 17:19:07
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/jenv.rb
License: MIT
==> Options
--HEAD
	Install HEAD version
==> Caveats
To activate jenv, add the following to your /Users/dxia/.bash_profile:

  export PATH="$HOME/.jenv/bin:$PATH"
  eval "$(jenv init -)"
==> Analytics
install: 9,672 (30 days), 30,663 (90 days), 129,619 (365 days)
install-on-request: 9,670 (30 days), 30,635 (90 days), 128,558 (365 days)
build-error: 0 (30 days)
```

With fish set as default shell:

```
brew info jenv

jenv: stable 0.5.4, HEAD
Manage your Java environment
https://www.jenv.be/
/opt/homebrew/Cellar/jenv/0.5.4 (82 files, 72.3KB) *
  Built from source on 2021-05-07 at 17:19:07
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/jenv.rb
License: MIT
==> Options
--HEAD
	Install HEAD version
==> Caveats
To activate jenv, run the following commands:

  echo 'status --is-interactive; and source (jenv init -|psub)' >> ~/.config/fish/config.fish"
  ln -sf /opt/homebrew/opt/jenv/libexec/fish/jenv.fish ~/.config/fish/functions/jenv.fish"
  ln -sf /opt/homebrew/opt/jenv/libexec/fish/export.fish ~/.config/fish/functions/export.fish"

==> Analytics
install: 9,672 (30 days), 30,663 (90 days), 129,619 (365 days)
install-on-request: 9,670 (30 days), 30,635 (90 days), 128,558 (365 days)
build-error: 0 (30 days)
```

1: https://github.com/jenv/jenv/blob/9bbc5ebfe6f38252a1a0f28897c7ae52e6a483a0/README.md
@davidxia
Copy link
Contributor Author

davidxia commented May 10, 2021

@carlocab Thanks for reviewing. Let me know any next steps required from me. If none, would be great to have this merged in.

Formula/jenv.rb Outdated Show resolved Hide resolved
@davidxia
Copy link
Contributor Author

Bumping to see what else I can do to move this forward. It seems like this thread is the only unresolved one? #76864 (comment)

cap10morgan
cap10morgan previously approved these changes Jun 2, 2021
Copy link
Contributor

@cap10morgan cap10morgan left a comment

Choose a reason for hiding this comment

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

I started to create a PR with more or less these same changes and then found this. This all works for me and is a big improvement over the current (broken) instructions. Thanks @davidxia!

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

I don't mind merging this, subject to one suggestion, and pending the answer to one question.

Formula/jenv.rb Outdated Show resolved Hide resolved
Formula/jenv.rb Outdated Show resolved Hide resolved
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
Copy link
Contributor

@cap10morgan cap10morgan left a comment

Choose a reason for hiding this comment

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

Hopefully have the last part in here as adoptable suggestions (in case that's helpful; feel free to ignore if not).

Formula/jenv.rb Outdated Show resolved Hide resolved
Formula/jenv.rb Show resolved Hide resolved
Formula/jenv.rb Outdated Show resolved Hide resolved
@carlocab carlocab removed the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Jun 2, 2021
Co-authored-by: Wes Morgan <github@wesmorgan.me>
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Great, I think we can merge this soon. Let's just try to clean up the whitespace a bit; I don't think we need this much.

Formula/jenv.rb Outdated Show resolved Hide resolved
Formula/jenv.rb Outdated Show resolved Hide resolved
Formula/jenv.rb Outdated Show resolved Hide resolved
@carlocab carlocab changed the title jenv: change caveat for fish shell jenv: install fish function symlinks Jun 2, 2021
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@cap10morgan
Copy link
Contributor

FWIW the old title of this PR is still appropriate for it. Or at least both effects are pretty important here (i.e. fixing the fish caveat instructions and installing the functions in the appropriate place).

@carlocab carlocab merged commit 268da91 into Homebrew:master Jun 2, 2021
@carlocab
Copy link
Member

carlocab commented Jun 2, 2021

Thanks, @davidxia, @cap10morgan.

@davidxia davidxia deleted the jenv-caveat-fish branch June 3, 2021 14:18
@github-actions github-actions bot added the outdated PR was locked due to age label Jul 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 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.

jenv doctor incorrect fix for "jenv not loaded in fish"
5 participants