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

Add Homebrew support #1042

Merged
merged 17 commits into from
Mar 7, 2022
Merged

Conversation

helpermethod
Copy link
Member

@helpermethod helpermethod commented Feb 1, 2022

  • add JReleaser Templates for the Homebrew Tap Formula and README
  • enable Homebrew packager in JReleaser's configuration
  • create a Personal Access Token with repo permissions to use as JRELEASER_HOMEBREW_GITHUB_TOKEN
  • add sdkman_selfupdate_feature flag to disable selfupdate functionality (including auto_update)
  • keep sdkman_selfupdate_enable flag for backwards-compatibility
  • introduce sdkman_auto_update flag for enabling/disabling auto_update (replaces sdkman_selfupdate_enable)
  • add tests to make sure that any combination of old / new flags works as expected

@aalmiray
Copy link
Contributor

aalmiray commented Feb 1, 2022

Watch out for jreleaser/jreleaser#702 when upgrading to JReleaser v1.0.0-M2 or later. Templates will use {{distributionExecutableUnix}} instead of {{distributionExecutable}}.

@helpermethod
Copy link
Member Author

helpermethod commented Feb 3, 2022

@marc0der Removing the sdkman-selfupdate.sh file as a post-install step is probably a little bit extreme but simplifies certain things. I'm still not sure if this is the best way 😆.

Alternatively we could introduce another flag for disabling the selfupdate functionality, and hope that no one switches it on.

@helpermethod helpermethod marked this pull request as ready for review February 6, 2022 21:04
@helpermethod
Copy link
Member Author

Hi @marc0der, I've added some tests to make sure that the old behaviour doesn't break.

Copy link
Member

@marc0der marc0der left a comment

Choose a reason for hiding this comment

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

Looks good so far. We shouldn't forget to add the new config to the sdkman-hooks service as mentioned.

src/main/bash/sdkman-init.sh Outdated Show resolved Hide resolved
src/test/groovy/sdkman/specs/SelfupdateSpec.groovy Outdated Show resolved Hide resolved
@helpermethod
Copy link
Member Author

@marc0der With the hook updates promoted to stable, I wonder if

# Attempt upgrade after all is done
if [[ "$COMMAND" != "selfupdate" && "$sdkman_selfupdate_feature" == "true" && ("$sdkman_auto_update" == "true" || "$sdkman_selfupdate_enable" == "true") ]]; then
	__sdkman_auto_update "$SDKMAN_REMOTE_VERSION" "$SDKMAN_VERSION"
fi

the sdkman_selfupdate_enable check here is actually needed? I guess the selfupdate hook will take care of replacing it, right?

Would be nice if we could get rid of that OR expression.

@marc0der
Copy link
Member

marc0der commented Mar 7, 2022

@helpermethod You are right. The selfupdate should take care of it, provided we deploy the hooks before the CLI. That said, I suggest leaving it in until both have been deployed, then removing it in a tiny follow-up PR once the dust has settled.

@helpermethod
Copy link
Member Author

Too late, already removed it 😂

@marc0der
Copy link
Member

marc0der commented Mar 7, 2022

@helpermethod You are too fast 😆 You probably need to update the SdkmanBashEnvBuilder and SelfupdateSpec too which still refer to the old variable.

@helpermethod
Copy link
Member Author

Removed the selfupdate_enable flag from the data table.

@helpermethod
Copy link
Member Author

Oh, and before we finally do the release I need to drop the old SDKMAN! Homebrew repository.

@marc0der
Copy link
Member

marc0der commented Mar 7, 2022

Okay, are you happy for me to merge that so we can test it in the beta channel?

@helpermethod
Copy link
Member Author

Let's go!

@marc0der marc0der merged commit f61702c into sdkman:master Mar 7, 2022
@helpermethod helpermethod deleted the homebrew-publication branch March 8, 2022 08:41
felipecrs added a commit to felipecrs/dotfiles that referenced this pull request Mar 18, 2022
Because some breaking changes were introduced in sdkman/sdkman-cli#1042.
@felipecrs
Copy link
Contributor

This was a breaking change for me, sad it wasn't tagged as one. I think it should be avoided to perform breaking changes without bumping the major version (like from 5 to 6).

felipecrs/dotfiles@5ba2f2b

@helpermethod
Copy link
Member Author

Sorry to hear 😥!

The sdk selfupdate command will automatically perform the migration, so for most users it should be a backwards-compatible change.

That's why we didn't bump the major version.

@felipecrs
Copy link
Contributor

felipecrs commented Mar 19, 2022

That's ok, it's not the first time anyway.

#1069 kind of prevents it (in the sense that if someone didn't touch the default configuration, it will be handled by the migration). But only in that case.

@felipecrs
Copy link
Contributor

By the way, docs are missing the new sdkman_auto_update:

https://sdkman.io/usage#config

r-darwish added a commit to r-darwish/topgrade that referenced this pull request Jun 18, 2022
* Ensure `selfupdate` is enabled for SDKMAN!

This subcommand is unavailable when the `sdkman_selfupdate_feature`
option is disabled, as is the case when SDKMAN! is installed via
Homebrew.

sdkman/sdkman-cli#1042

* Fix macOS build; simplify

Co-authored-by: Roey Darwish Dror <[email protected]>
ahutsunshin added a commit to ahutsunshin/topgrade that referenced this pull request Nov 17, 2022
* Ensure `selfupdate` is enabled for SDKMAN!

This subcommand is unavailable when the `sdkman_selfupdate_feature`
option is disabled, as is the case when SDKMAN! is installed via
Homebrew.

sdkman/sdkman-cli#1042

* Fix macOS build; simplify

Co-authored-by: Roey Darwish Dror <[email protected]>
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.

4 participants