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

formula_auditor: do not allow depending on aliases #10115

Merged
merged 2 commits into from
Dec 24, 2020

Conversation

bayandin
Copy link
Member

@bayandin bayandin commented Dec 23, 2020

  • 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.
  • 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?
  • Have you successfully run brew man locally and committed any changes?

This PR makes dependency audit more strict and prohibits using aliases (at all) in depends_on in homebrew-core:

  # Before both of these options were allowed (`openjdk@15` is an alias to `openjdk`:)
  depends_on "openjdk@15"
  depends_on "openjdk"
  
  # After it allows only:
  depends_on "openjdk"

Motivation:

  • We'd like formulae to use the latest version of dependencies (when possible). When a new version of the formula appears we'd like all the depending on it formulae to start using it. So having openjdk@15 as a dependency will not allow us making this switch (to the next openjdk@16) automatically and will require manual formula tuning. Also, we don't have such kind of examples in homebrew-core;
  • Currently, dependency audit doesn't work for [email protected] and [email protected] (these are special cases when an unversioned formula is an alias to versioned one). And it was easier to make this audit more strict than add tuning for these special cases 🙂

Existing violations fixed:

@BrewTestBot
Copy link
Member

Review period will end on 2020-12-24 at 15:02:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 23, 2020
@bayandin bayandin force-pushed the do-not-depend-on-aliases branch from f5ecb62 to 1e68ce5 Compare December 23, 2020 23:29
@@ -235,8 +235,7 @@ def audit_deps
problem "Dependency '#{dep.name}' was renamed; use new name '#{dep_f.name}'."
end

if self.class.aliases.include?(dep.name) &&
dep_f.core_formula? && !dep_f.versioned_formula?
if self.class.aliases.include?(dep.name) && dep_f.core_formula?
Copy link
Member

Choose a reason for hiding this comment

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

We're checking here whether the formula being depended on is a core formula but I think we should be checking for both the dependency and the main formula because depending on a versioned alias is actively desirable outside of homebrew/core. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right!
I've actually misread the condition, in my head exactly what you're saying 😄
I'll put this check after next unless @core_tap. And then we don't really need to check dep_f.core_formula? because formulae from homebrew-core can depend only on formulae from homebrew-core

@MikeMcQuaid
Copy link
Member

  • Currently, dependency audit doesn't work for [email protected] and [email protected] (these are special cases when an unversioned formula is an alias to versioned one). And it was easier to make this audit more strict than add tuning for these special cases 🙂

Does this mean they are a false positive, false negative or produce an error? Thanks!

@bayandin
Copy link
Member Author

bayandin commented Dec 24, 2020

Does this mean they are a false positive, false negative or produce an error? Thanks!

It's a false negative, I guess.

Current logic says: a formula can't depend on aliases unless it's an alias to a versioned formula. So it allows aliases to versioned formulae (openssl -> [email protected]). And I suggest (in this PR) not to allow it at all

@MikeMcQuaid
Copy link
Member

It's a false negative, I guess.

Assuming that means "we should complain but we don't": fine with me 👍🏻

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 24, 2020
@BrewTestBot
Copy link
Member

Review period ended.

@bayandin bayandin merged commit 5a7b70a into Homebrew:master Dec 24, 2020
@bayandin bayandin deleted the do-not-depend-on-aliases branch December 24, 2020 18:13
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 24, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 24, 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