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

Refactor some dependency handling to fix various API dependency issues #15566

Merged
merged 5 commits into from Jul 4, 2023

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Jun 19, 2023

This PR tries to tackle several of the "harder" issues involving API dependencies. Basically the stuff that required deeper refactoring rather than a hack-on fix.

There were several cases where the dependency list was incorrect. Usually this erred on the side of too many dependencies rather than too little, so hasn't seen widespread bug reports beyond a few comments here and there.

Notably:

  • The dependency list had parts that varied depending on the machine that generated the API JSON rather than the consumer.
  • Stable-only or HEAD-only dependencies were not correctly considered.
  • uses_from_macos "example", since: ... did not work correctly.

This changes in this PR are:

  • Removed TapDependency (this makes the rest of the PR a bit simpler), folding it into Dependency
  • Refactored uses_from_macos handling. Instead of being a conditional depends_on, it now consistently adds a UsesFromMacOSDependency on all platforms with the checks now taking place in installed?. This provides a more consistent behaviour on all platforms, and has a bonus of reducing the bloat of the variations part of the JSON.
  • Introduced an internal auto tag, which is now applied to all dependencies not explicitly added via a depends_on. This fixes an issue where some of the "if needed" deps leaked from the machine that generated the API JSON. For example, if a formula uses xz and the machine that generated the API JSON did not have xz, it will now no longer appear on the API dependency list and instead be determined by the API consumer.
  • Changed the API hash and Formulary to support all these changes. Some fields in the hash were deprecated as a result as some changes were unavoidably backwards incompatible given the basic array of strings we had before simply does not have the flexibility to store the additional information added in this PR like a hash/dictionary does. (Requirements already used a hash so the existing field worked there.)

This is draft because rspec tests need adjusting and basic commands are probably very broken as is. But the general implementation as a whole should be done, with just bug/typo fixes needed.

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.

Looking good so far!

My main concern is the migration path for API consumers here and avoiding making API consumers having to rewrite their code for a change that none of them having really complained about.

Somewhat relatedly: I've been thinking for a little while that I think there could be benefit in a JSON output/API subset that is strictly what we need and not what API consumers do. Now that we have formula.jws.json and cask.jws.json: I 100% guarantee that no-one except us is decoding and using these files except us so they seem like a very good candidate for slimming down API fields to the bare minimum that we need/consume to make those files smaller.

For other cases e.g. brew info --json output and the formula.json files: I'm tempted to say we don't deprecate those old fields and just leave them around indefinitely. They aren't really size-sensitive and not breaking our (pretty much unversioned) public APIs is much more appealing than slimming down these files.

Thanks again for the PR!

Library/Homebrew/dependable.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula_auditor.rb Outdated Show resolved Hide resolved
Library/Homebrew/formulary.rb Outdated Show resolved Hide resolved
@carlocab
Copy link
Member

Will have a look at this PR later, but since we're looking at refactoring dependency handling:

Some of the "auto" dependencies (e.g. xz when the source tarball is a .tar.xz, or curl when the url has using: :homebrew_curl) get added to the build environment by superenv, which leads to opportunistic linkage (see icecast.rb and vorbis-tools.rb for hacks we've used to try to avoid this). It would be nice if we could get rid of this (if this PR doesn't handle that already).

@Bo98
Copy link
Member Author

Bo98 commented Jun 21, 2023

Thanks for reminding me. I knew there was another reason for tagging those dependencies as this wasn't the first time I had the idea to do that but I had forgotten what that other reason was.
I will indeed make that change.

@MikeMcQuaid
Copy link
Member

It would be nice if we could get rid of this (if this PR doesn't handle that already).

Agreed 👍🏻

I will indeed make that change.

🎉

@Bo98
Copy link
Member Author

Bo98 commented Jun 22, 2023

Some of the "auto" dependencies (e.g. xz when the source tarball is a .tar.xz, or curl when the url has using: :homebrew_curl) get added to the build environment by superenv, which leads to opportunistic linkage (see icecast.rb and vorbis-tools.rb for hacks we've used to try to avoid this). It would be nice if we could get rid of this (if this PR doesn't handle that already).

fd411c2 will probably handle this (untested).

Though that won't necesssarily stop opportunistic linkage from simply being installed, should any formula do that - but that's not specific to implicit dependencies. The change should however stop it from being added to the superenv variables.

@MikeMcQuaid
Copy link
Member

@Bo98 been following along with this but just FYI I'll hold off another proper review until it's non-draft. Feel free to nudge me in Slack if I miss that!

@Bo98 Bo98 force-pushed the dep-refactor branch 2 times, most recently from 828654a to 44ea6fb Compare July 3, 2023 02:19
@Bo98 Bo98 marked this pull request as ready for review July 3, 2023 02:20
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.

Looks great, thanks again @Bo98!

raise ArgumentError, "Dependency must have a name!" unless name

@name = name
@tags = tags
@env_proc = env_proc
@option_names = option_names

@tap = Tap.fetch(name.rpartition("/").first) if name.match?(HOMEBREW_TAP_FORMULA_REGEX)
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking: given how often we do this all over the place: feels like we should have a better accessor for this sort of thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably.

I've tweaked this slightly in the meantime to:

@tap = Tap.fetch(Regexp.last_match(1), Regexp.last_match(2)) if name =~ HOMEBREW_TAP_FORMULA_REGEX

@MikeMcQuaid MikeMcQuaid merged commit a5a728a into Homebrew:master Jul 4, 2023
24 checks passed
@Bo98 Bo98 deleted the dep-refactor branch July 4, 2023 13:58
@github-actions github-actions bot added the outdated PR was locked due to age label Aug 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2023
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.

None yet

3 participants