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

Improve suggestions for depends_on_java #11290

Merged

Conversation

joewiz
Copy link
Contributor

@joewiz joewiz commented Apr 29, 2021

The current behavior of the depends_on_java stanza has 3 shortcomings:

  1. The caveat is hardcoded to treat a dependency on Java 11 as a dependency on the latest Java. That is no longer a correct assumption, as Java 16 is the latest version.
  2. When a caveat lists a specific version (without the + quantifier), Homebrew advises users to install that version of adoptopenjdk from cask-versions. However, cask-versions only has adoptopenjdk8, not the full array of versions (8-16, etc.). Only the adoptopenjdk/openjdk tap has the full array.
  3. The caveat doesn't handle an array of versions (e.g., depends_on_java ["8", "11"]) particularly gracefully. The URL it constructs outputs the array instead of per-version URLs (i.e., adoptopenjdk["8", "11"] instead of adoptopenjdk8 and adoptopenjdk11).

This PR fixes issues the first two issues above:

  1. Rather than having to track Java stable releases, this PR removes the hardcoded reference to Java 11. Any cask with the stanza using 11+ (or 12+, for example), will be told how to install the latest version:

    ... requires Java 11+. You can install the latest version with:

    brew install --cask adoptopenjdk
    
  2. However, a stanza without the + will be told to install a specific version - which now conforms to https://github.com/Homebrew/homebrew-cask/pull/54074/files:

    ... requires Java 11. You can install it with:

    brew tap adoptopenjdk/openjdk
    brew install --cask adoptopenjdk11
    
  3. Unfortunately, my Ruby skills are too limited to fix the 3rd issue above - the presentation of arrays. Ideally, we could take the array described by @vitorgalvao in depends_on :java homebrew-cask#16383 and output sample installation commands for each version, instead of what would appear for the exist-db cask:

    exist-db requires Java ["8", "11"]. You can install it with:

    brew tap adoptopenjdk/openjdk
    brew install --cask adoptopenjdk["8", "11"]
    

    A better output would be:

    exist-db requires Java 8 or 11. You can install it with:

    brew tap adoptopenjdk/openjdk
    

    and one of the following, depending on the desired version:

    brew install --cask adoptopenjdk8
    brew install --cask adoptopenjdk11
    

    I would welcome anyone with Ruby skills to contribute a fix to this issue, either in this PR or in a follow-on PR.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
    Note: My apologies. I lack the Ruby skills to write tests for this PR.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
    Note: regarding brew tests, running it on my branch failed (see brew config output below), but the log does not appear to fail due to anything changed in my PR. See the console log: brew-tests-log.txt.zip

My brew config:

HOMEBREW_VERSION: 3.1.5-2-g05fe138
ORIGIN: https://github.com/Homebrew/brew
HEAD: 05fe138bae3bde404e5beb05482952205d9df65c
Last commit: 6 minutes ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: db2cd52c78fc9ba01fa8ff65e9bdc6e2028c4b7d
Core tap last commit: 79 minutes ago
Core tap branch: master
HOMEBREW_PREFIX: /usr/local
HOMEBREW_CASK_OPTS: []
HOMEBREW_EDITOR: nano
HOMEBREW_MAKE_JOBS: 16
Homebrew Ruby: 2.6.3 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: 16-core 64-bit dunno
Clang: 12.0 build 1205
Git: 2.31.1 => /usr/local/bin/git
Curl: 7.64.1 => /usr/bin/curl
macOS: 11.3-x86_64
CLT: 12.5.0.22.9
Xcode: 12.5

Java 11 is no longer the current stable build, so a dependency on “11” should not be treated as a dependency on the latest Java.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @joewiz ❤️ ! I agree this behaviour could be improved but just to warn you there's likely to be a bit of back-and-forth to get there.

Library/Homebrew/cask/dsl/caveats.rb Show resolved Hide resolved
@@ -114,15 +114,16 @@ def eval_caveats(&block)
#{@cask} requires Java. You can install the latest version with:
brew install --cask adoptopenjdk
EOS
elsif java_version.include?("11") || java_version.include?("+")
elsif java_version.include?("+")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why 11 was specified here already and if there's another value/values that should be here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the inception of the file, this value always tracked the current stable release of Java - advancing to 9, 10, and finally 11. While Java continued to release major versions since then, Homebrew didn't track these changes. One option would be to resume tracking it - but in my opinion, any value with a + quantifier implies "current stable release", so there's really no benefit to tracking the current version. The real purpose of + is to establish a minimum compatible version and to encourage users to install the current version.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth parsing this from the Java cask if it's available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that would be a nice approach (although implementing such parsing of the Java cask is beyond my abilities, I'm afraid).

<<~EOS
#{@cask} requires Java #{java_version}. You can install the latest version with:
brew install --cask adoptopenjdk
EOS
else
<<~EOS
#{@cask} requires Java #{java_version}. You can install it with:
brew install --cask homebrew/cask-versions/adoptopenjdk#{java_version}
brew tap adoptopenjdk/openjdk
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to recommend third-party taps not part of the Homebrew organisation for security reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Homebrew/homebrew-cask#54074, @claui, @vitorgalvao, and @commitay discussed this in some detail and approved this approach.

Copy link
Member

Choose a reason for hiding this comment

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

In Homebrew/homebrew-cask#54074, @claui, @vitorgalvao, and @commitay discussed this in some detail and approved this approach.

There is a huge difference between saying “we don’t support other versions of this software; if you want them you can get check this external tap” (what Homebrew/homebrew-cask#54074 does) and “to install this piece of software you have to install software from a third-party tap” (what this does).

The former gives you a choice on the tool itself—you have to understand what it is. The latter is a requirement of a dependency—you may not understand the implication, only that it’s stopping you from getting something else.

I agree with @MikeMcQuaid, with one clarification:

We don't want to recommend require third-party taps not part of the Homebrew organisation for security reasons.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. That said: I don't think we should recommend or require them (outside of https://docs.brew.sh/Interesting-Taps-and-Forks) because your typical user will just do what the instructions say to fix their problem which puts an onus on us to ensure that their security meets our standards (which we cannot do).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vitorgalvao I understand the concern. From the perspective of a maintainer of a cask that currently is not yet confirmed to work with the version of Java that the Homebrew organization offers, I would be in favor of a solution that allows cask maintainers to advise users how to safely use the cask. We are actively working on ensuring compatibility with the current version of Java (16), but in the meantime, a "caveat" seems like the right medicine (and certainly better than nothing). Perhaps rather than endorsing a third-party tap, this language could be used:

We don't support other versions of this dependency. Consult the project's documentation to locate a third-party tap or source for the dependency.

Would that satisfy the concern about not endorsing third-party taps for security reasons?

Copy link
Member

Choose a reason for hiding this comment

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

From the perspective of a maintainer of a cask that currently is not yet confirmed to work with the version of Java that the Homebrew organization offers

Is it known to be broken or just not confirmed to work?

We don't support other versions of this dependency.

So: we don't support any of the "official" Homebrew tap versions at all? In that case I'd say this cask should probably be removed from homebrew-cask entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see the official homebrew/cask-versions contains adoptopenjdk8, which the exist-db cask is certainly compatible with.

Given the direction that this discussion has taken—i.e. toward removing mentions of third party casks—I will submit a PR updating the exist-db cask's caveat to remove mention of "11", to read: depends_on_java: "8".

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Paging @Homebrew/cask to discuss recommendation of other taps.

@joewiz
Copy link
Contributor Author

joewiz commented May 3, 2021

@MikeMcQuaid @vitorgalvao I've now updated the PR to remove the commit that introduced the mention of the 3rd party tap. All that remains is the commit removing the hardcoded reference to java "11". Certainly, there is no Homebrew organization edition of Java 11, so this PR is a step forward.

But it falls short of @MikeMcQuaid's good suggestion:

Would it be worth parsing this from the Java cask if it's available?

In light of this, I'd understand if you decide to close this PR in favor of a more complete solution. Whatever you all think is the best route is fine with me.

Thank you!

@MikeMcQuaid MikeMcQuaid merged commit 5bf9d1b into Homebrew:master May 4, 2021
@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @joewiz!

@joewiz joewiz deleted the fix-depends-on-java-version-parsing branch May 4, 2021 14:08
@joewiz
Copy link
Contributor Author

joewiz commented May 4, 2021

@MikeMcQuaid Thank you for reviewing and merging it—and thanks to you, @vitorgalvao, and all of the Homebrew maintainers for all of your work building this amazing tool and community!

@github-actions github-actions bot added the outdated PR was locked due to age label Jun 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants