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

Freeze formula definition once first instance is created #13753

Merged
merged 7 commits into from
Sep 1, 2022

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Aug 25, 2022

This PR implements safety mechanisms to avoid hitting the unsupported behaviour of modifying a formula's definition (e.g. adding a dependency) after the first instance has been created. Note that you will still have the ability to do so if such changes are applied to only that particular instance of the formula (typically involving private API). Only changes to the global definition on the class-level is prevented, as those would have an undefined effect on existing instances.

Each commit can be separated into a separate PR, but the order matters as each commit depends on the previous one.

Details on each one, in reverse order:

  • 22ccede - Freeze formula definition once first instance is created
    • What this does is call self.class.freeze at the begininning of initialize.
    • Since we want to prevent modification of SoftwareSpec etc too, I've made it propagate the freeze request down.
    • We already dup the SoftwareSpecs when create the instance, which means the spec stored at instance level will be unfrozen. I've added code to also propagate the dup down too so everything on instance level is unfrozen and cannot propagate back up to the global definition.
    • A part of this involved moving lazily-initialized variables to a self.inherited method, which act's effectively like a class-level constructor.
  • 7e825af - test: avoid improper, late usage of formula DSL
    • Fixes multiple tests that abused late formula modifications like Formula["foo"].class.depends_on "bar", which this PR was aiming to prevent being possible
    • This contains some overlap with install glibc/gcc automatically if too old. #13577, but the PRs merged in any order.
  • 39a83ee - formula: require instances to use a subclass
    • Because I'm using self.inherited, some variables are no longer initialised on the Formula base class. Calling Formula.new on this base class never really worked anyway since it doesn't make sense so I made it an explicit error when this action is performed.
  • df94985 - requirement: require instances to use a subclass
    • Apply the same logic to Requirement so I can share the logic of BuildEnvironment::DSL better.
    • This technically worked without errors before - but it didn't make any sense since the requirement would have no satisfy conidition so it was a no-op requirement.
  • fe87f36 - cask_dependent: fix incorrect requirement logic
    • The change requiring Requirement instances to use a subclass revealed a bug: the logic in CaskDependent was incorrect in a few ways (e.g. it displayed "x86_64" for a formula requiring arm64).
  • 0a780b9 - requirement: improve name detection of anonymous subclasses
    • There's logic to use the required cask name if the requirement itself doesn't have a name. But this never ever got used because it would prioritise a class.to_s which is typically not human-readable.
    • This name is in brew deps --include-requirements

@BrewTestBot
Copy link
Member

Review period will end on 2022-08-26 at 02:12:21 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 25, 2022
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.

Great work here! Would like to see a bunch more comments (particularly explaining why the code is doing what it is doing rather than what it is doing) and there's a brew typecheck failure but otherwise this looks great!

Library/Homebrew/build_environment.rb Show resolved Hide resolved
Library/Homebrew/cask_dependent.rb Outdated Show resolved Hide resolved
Library/Homebrew/extend/module.rb Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/requirement.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Outdated Show resolved Hide resolved
Library/Homebrew/formula.rb Show resolved Hide resolved
Library/Homebrew/requirement.rb Show resolved Hide resolved
Library/Homebrew/dependency_collector.rb Show resolved Hide resolved
Library/Homebrew/dependency_collector.rb Show resolved Hide resolved
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 26, 2022
@BrewTestBot
Copy link
Member

Review period ended.

@Bo98
Copy link
Member Author

Bo98 commented Aug 31, 2022

Had to do 88a8def to get brew typecheck working. If you don't want sig to bleed through as DSL then we could probably do it in formula.rbi instead.

@MikeMcQuaid MikeMcQuaid merged commit f788277 into Homebrew:master Sep 1, 2022
@MikeMcQuaid
Copy link
Member

Thanks @Bo98!

@carlocab
Copy link
Member

carlocab commented Sep 1, 2022

This seems to be causing issues for resources defined inside on_os blocks? See Homebrew/homebrew-core#109417.

@Bo98
Copy link
Member Author

Bo98 commented Sep 1, 2022

I guess we have no test coverage of that? Which is surprising. Will take a look.

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Sep 4, 2022
Defining methods inside `test` blocks is no longer supported as of
Homebrew/brew#13753.

Fixes a CI failure spotted in Homebrew#106925:

      Error: scry: failed
      An exception occurred within a child process:
        FrozenError: can't modify frozen class
carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Sep 4, 2022
Defining methods inside `test` blocks is no longer supported as of
Homebrew/brew#13753.

Fixes a CI failure spotted in #106925:

      Error: scry: failed
      An exception occurred within a child process:
        FrozenError: can't modify frozen class
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Sep 4, 2022
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Sep 4, 2022
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Sep 4, 2022
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Sep 4, 2022
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Sep 4, 2022
carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Sep 5, 2022
carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Sep 5, 2022
carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Sep 5, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 2, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2022
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

4 participants