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

rubocops: add rubocop for old bottle syntax #10453

Merged
merged 3 commits into from
Feb 4, 2021

Conversation

SeekingMeaning
Copy link
Contributor

@SeekingMeaning SeekingMeaning commented Jan 28, 2021

  • 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?

Part 3 of #9315

brew style --fix homebrew/core: SeekingMeaning/homebrew-core@tmp-new-bottle-syntax

5467 files inspected, 36416 offenses detected, 36416 offenses corrected

Before:

  bottle do
    rebuild 4
    sha256 "e87da2b47386fc7e3c6f20b3ff90c4bbe37b9e0aaa884440ffa216492dbc150b" => :big_sur
    sha256 "8ac02041dbec3966b6a695dfc4215b90b9e331ae6eb8c6698cbbfa0175154c9f" => :arm64_big_sur
    sha256 "82e64b2008971430d160a3f564e32593e98fb55c43d7748c7deb9d6f546e1102" => :catalina
    sha256 "8ca49b4797277f79e87e48ab4c6794601b64d1dde35b9eac556d4153b8237a51" => :mojave
  end

After:

  bottle do
    rebuild 4
    sha256 big_sur: "e87da2b47386fc7e3c6f20b3ff90c4bbe37b9e0aaa884440ffa216492dbc150b"
    sha256 arm64_big_sur: "8ac02041dbec3966b6a695dfc4215b90b9e331ae6eb8c6698cbbfa0175154c9f"
    sha256 catalina: "82e64b2008971430d160a3f564e32593e98fb55c43d7748c7deb9d6f546e1102"
    sha256 mojave: "8ca49b4797277f79e87e48ab4c6794601b64d1dde35b9eac556d4153b8237a51"
  end

@BrewTestBot
Copy link
Member

Review period will end on 2021-01-29 at 20:40:25 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 28, 2021
@Rylan12
Copy link
Member

Rylan12 commented Jan 28, 2021

A few initial thoughts (haven't gone through the code at all).

Is it possible/would it make sense to align the hashes? It would cause the bottle block to go from

bottle do
  sha256 cellar: :any, big_sur: "b95aa332dfc7c6dfb5e86fd30068f78e2cf87ee0232e5bef0adddae8215f543d"
  sha256 cellar: :any, arm64_big_sur: "b480ed6baf10880f61b5a3097fb0921d44466857e1dde53a09e2ae4e378b1a8c"
  sha256 cellar: :any, catalina: "8ec66cf6faa310712767efc3022fdd16568a79234439f64bf579acb628f893bc"
  sha256 cellar: :any, mojave: "245a43a59c57f83848e7382974bb80a46eac1d53bcaefb1bdebd1f85107d4169"
  sha256 cellar: :any, high_sierra: "30548658b43cf66979f2756680fbb32d3c19c967e478ceea22d07f536b22bbce"
  sha256 cellar: :any, sierra: "f822b4dbab4a15b889316b89248c7b4d15d6af9dc460bf209b9425b0accb7fa3"
  sha256 cellar: :any, el_capitan: "3f912f6f1ce6c586128ebde29756c883b89409e652ca7aa9a29a773c2d4d0915"
  sha256 cellar: :any, yosemite: "5b969eb38b90a3e31869586df9d62e59d359212b16c6a270aee690dd67caa491"
end

to

bottle do
  sha256 cellar: :any, big_sur:       "b95aa332dfc7c6dfb5e86fd30068f78e2cf87ee0232e5bef0adddae8215f543d"
  sha256 cellar: :any, arm64_big_sur: "b480ed6baf10880f61b5a3097fb0921d44466857e1dde53a09e2ae4e378b1a8c"
  sha256 cellar: :any, catalina:      "8ec66cf6faa310712767efc3022fdd16568a79234439f64bf579acb628f893bc"
  sha256 cellar: :any, mojave:        "245a43a59c57f83848e7382974bb80a46eac1d53bcaefb1bdebd1f85107d4169"
  sha256 cellar: :any, high_sierra:   "30548658b43cf66979f2756680fbb32d3c19c967e478ceea22d07f536b22bbce"
  sha256 cellar: :any, sierra:        "f822b4dbab4a15b889316b89248c7b4d15d6af9dc460bf209b9425b0accb7fa3"
  sha256 cellar: :any, el_capitan:    "3f912f6f1ce6c586128ebde29756c883b89409e652ca7aa9a29a773c2d4d0915"
  sha256 cellar: :any, yosemite:      "5b969eb38b90a3e31869586df9d62e59d359212b16c6a270aee690dd67caa491"
end

I don't feel super strongly about this but it may add some uniformity if people like that.

Separately, what's the plan for updating these? I have a bad feeling about doing a single commit that changes 5,465 files or even 5,465 commits that modify one file each. This means that the next brew update will take quite a while, no?

@SeekingMeaning
Copy link
Contributor Author

Separately, what's the plan for updating these? I have a bad feeling about doing a single commit that changes 5,465 files or even 5,465 commits that modify one file each. This means that the next brew update will take quite a while, no?

I suppose #7867 provides a precedent: one commit for each formula

Copy link
Member

@Rylan12 Rylan12 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 to me. I do think we should have some tests for this if possible.

sha256_pair = sha256_pairs.first
sha256_key = sha256_pair.key
sha256_value = sha256_pair.value
next unless sha256_value.is_a?(AST::SymbolNode)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
next unless sha256_value.is_a?(AST::SymbolNode)
next unless sha256_value.sym_type?

Are these equivalent? If so, I think I prefer sym_type? (although either is fine)

Copy link
Contributor Author

@SeekingMeaning SeekingMeaning Jan 29, 2021

Choose a reason for hiding this comment

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

Yes, I'm pretty sure they're equivalent. This specific file probably isn't affected, but generally Sorbet doesn't like the *_type? methods. Somewhat off-topic, but it'd be really cool if they ever eventually add support for this (#sym_type? being equivalent to #is_a?(SymbolNode))

@SeekingMeaning
Copy link
Contributor Author

SeekingMeaning commented Jan 29, 2021

I do think we should have some tests for this if possible.

Ah right, I keep forgetting to do this 😄 Even with the checklist

@iMichka
Copy link
Member

iMichka commented Jan 29, 2021

One question: we reuse the cellar we had and put it on each line. This does not take into account cases like this:
Homebrew/homebrew-core@afef106

It's not a big deal because it does not change anything compared to the old situation, but reformatting everything does not improve this. You actually need to rebuild the whole formula to get the right output.

@jonchang
Copy link
Contributor

For the tests, I believe you have split it so each it block only contains an expect_offense followed by an expect_correction (at least that's how I had to write it when I refactored our formula rubocops)

@carlocab
Copy link
Member

carlocab commented Jan 29, 2021

This is really not nice to read:

bottle do
    sha256 big_sur: "cb2cb92999ff78e4f3ecfb57fca6782c481f6fe9abd5947e3f24569c3066c94b"
    sha256 cellar: :any, arm64_big_sur: "5970413cf75bce3e0908c0bc02fafe96d83559eec80e618e264638b007673efd"
    sha256 catalina: "dbdbf05a94e97c5acec62d09272dc407f42244a0d2546ac6f273843484142183"
    sha256 mojave: "99139c8d3d78914799034b4ce56aeefa106be643eff5ab2952b06eeff98231db"
end

Any thoughts on either (or both) of:

  • moving the :arm64_big_sur before or after the Intel macOS lines
  • specifying cellar: :default if it's not cellar: :any, etc, and then aligning the OS symbols (or we could just align them even with the blank space there?)

@MikeMcQuaid
Copy link
Member

  • moving the :arm64_big_sur before or after the Intel macOS lines

Fine with me, I'd suggest before.

  • specifying cellar: :default if it's not cellar: :any, etc, and then aligning the OS symbols (or we could just align them even with the blank space there?)

I'm not opposed to aligning. I'm slightly opposed to cellar :default but not hugely.

@iMichka
Copy link
Member

iMichka commented Jan 29, 2021

By the way, you could also have cellar: "/home/very/very/very/very/very/very/long/path" (if someone is playing with a non-default cellar path).

@MikeMcQuaid
Copy link
Member

I suppose #7867 provides a precedent: one commit for each formula

I think this would be fine to do all in one commit, for a rare change, because it's affecting pretty much every single formula.

I have no preference either way, though. For one-formula-per-commit you may find this useful: https://github.com/MikeMcQuaid/dotfiles/blob/master/bin/git-commit-each

@MikeMcQuaid
Copy link
Member

Also: if we change the style/indentation: let's also update the relevant dev-cmd/bottle.rb (and tests) code to ensure we always output in a format that's not going to be autocorrected into something else.

@SeekingMeaning SeekingMeaning added the in progress Maintainers are working on this label Jan 29, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 29, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@SeekingMeaning
Copy link
Contributor Author

SeekingMeaning commented Jan 29, 2021

Okay, so it looks like this is the general consensus:

  1. Align sha256 digests (or at least, no one seems opposed to it)
  2. Move ARM checksums to the front
  3. Update bottle do blocks in homebrew/core in a single commit
    • arm64_big_sur cellars will still be incorrect and ARM bottles need to be rebuilt to get the correct cellar

So if we assume a theoretical macOS 12 is named Grand Canyon, this is how a bottle block may (tentatively) look like:

bottle do
  sha256 cellar: :any, arm64_grand_canyon: "eae6eb45b833a50f13c19bb07d55f1bfffa60da9dadb855433f54b75c63beaf2"
  sha256 cellar: :any, arm64_big_sur:      "5970413cf75bce3e0908c0bc02fafe96d83559eec80e618e264638b007673efd"
  sha256               grand_canyon:       "f2fec3bcae901be1a6607884ed095a133670f86fe601bdb9b5c27205e9755ea2"
  sha256               big_sur:            "cb2cb92999ff78e4f3ecfb57fca6782c481f6fe9abd5947e3f24569c3066c94b"
  sha256               catalina:           "dbdbf05a94e97c5acec62d09272dc407f42244a0d2546ac6f273843484142183"
  sha256               mojave:             "99139c8d3d78914799034b4ce56aeefa106be643eff5ab2952b06eeff98231db"
end

Without checksums aligned:

bottle do
  sha256 cellar: :any, arm64_grand_canyon: "eae6eb45b833a50f13c19bb07d55f1bfffa60da9dadb855433f54b75c63beaf2"
  sha256 cellar: :any, arm64_big_sur: "5970413cf75bce3e0908c0bc02fafe96d83559eec80e618e264638b007673efd"
  sha256 grand_canyon: "f2fec3bcae901be1a6607884ed095a133670f86fe601bdb9b5c27205e9755ea2"
  sha256 big_sur: "cb2cb92999ff78e4f3ecfb57fca6782c481f6fe9abd5947e3f24569c3066c94b"
  sha256 catalina: "dbdbf05a94e97c5acec62d09272dc407f42244a0d2546ac6f273843484142183"
  sha256 mojave: "99139c8d3d78914799034b4ce56aeefa106be643eff5ab2952b06eeff98231db"
end

Also, considering that ARM will likely replace Intel, similar to how PowerPC was replaced, would it make sense to start adding a prefix like intel_ or x86_64_?

@SeekingMeaning
Copy link
Contributor Author

For the tests, I believe you have split it so each it block only contains an expect_offense followed by an expect_correction (at least that's how I had to write it when I refactored our formula rubocops)

I think it should be okay to do multiple calls of expect_offense and expect_correction in an it block

@MikeMcQuaid
Copy link
Member

  • Align sha256 digests (or at least, no one seems opposed to it)
  • Move ARM checksums to the front
  • Update bottle do blocks in homebrew/core in a single commit

These all seem good to me 👍🏻

  • arm64_big_sur cellars will still be incorrect and ARM bottles need to be rebuilt to get the correct cellar

Can you elaborate on this? Is it that they are incorrectly marked as cellar :any (or not)?

Also, considering that ARM will likely replace Intel, similar to how PowerPC was replaced, would it make sense to start adding a prefix like intel_ or x86_64_?

To future macOS versions: yeh, perhaps. I think we should avoid changing the naming of bottles after we've started shipping them for a given OS. My thinking is/was also that in a few years (<5?) we'll probably see a new macOS version that doesn't support Intel at all.

@Rylan12
Copy link
Member

Rylan12 commented Feb 1, 2021

  • Align sha256 digests (or at least, no one seems opposed to it)
  • Move ARM checksums to the front
  • Update bottle do blocks in homebrew/core in a single commit

👍 from me as well on these.

One other thing to do at some point (even if it's not in this PR) is to update the bottle docs.

@SeekingMeaning
Copy link
Contributor Author

Is it that they are incorrectly marked as cellar :any (or not)?

Yes


Dealing with some personal issues, so if anyone would like to take over this PR please feel free to do so

@Rylan12
Copy link
Member

Rylan12 commented Feb 2, 2021

I'm happy to take this up, but I'll need to wait for tomorrow (it's too late here).

I don't totally understand the arm bottle issue. Is it because the arm bottles have different cellars from the intel bottles, but since support for differing cellars is very new, the bottles may be incorrectly labeled? If so, that's not really a problem with this PR, right? That will require re-bottling. It sounds like moving forward with this (using the existing cellar line) and letting version-bump-bottling handle the updates is the best way to go?

Alternatively, I guess we could label all arm bottles as having the default cellar (meaning not cellar :any or cellar :any_skip_relocation) to minimize pouring into invalid cellars. This would inhibit those bottles that do work everywhere from being used until a new version is bottled, though.

@iMichka
Copy link
Member

iMichka commented Feb 2, 2021

Is it because the arm bottles have different cellars from the intel bottles, but since support for differing cellars is very new, the bottles may be incorrectly labeled?

I saw that. Before, this was hidden by the brew bottle --merge code logic, as there only one cellar line. I do not think the code that writes the cellar line does anything wrong: it just reads the value from the bottle.json file that is created by the bottling process. If you want to have a different cellar line, you need to fix the code that generates this json file, and understand what is going on.

If so, that's not really a problem with this PR, right?

Yes, this is unrelated to this PR. Linux bottles might also have different cellar lines, so I am not surprised there are differences on Mac too.

That will require re-bottling

It depends if the cellar line is legit or was wrongly generated due to a bug in brew. If you consider that a cellar line is wrong, you need to find out why and fix the bug in brew first, before starting to rebottle things.

@MikeMcQuaid
Copy link
Member

Dealing with some personal issues, so if anyone would like to take over this PR please feel free to do so

@SeekingMeaning Hope you're ok ❤️

I'm happy to take this up, but I'll need to wait for tomorrow (it's too late here).

@Rylan12 That'd be great!

If so, that's not really a problem with this PR, right? That will require re-bottling. It sounds like moving forward with this (using the existing cellar line) and letting version-bump-bottling handle the updates is the best way to go?

Agreed 👍🏻

@MikeMcQuaid
Copy link
Member

(Would like to land this for Homebrew 3.0.0)

@Rylan12 Rylan12 force-pushed the old-bottle-rubocop branch from 6e4bc20 to a632cbc Compare February 2, 2021 17:29
@Rylan12

This comment has been minimized.

@Rylan12

This comment has been minimized.

@Rylan12 Rylan12 force-pushed the old-bottle-rubocop branch 2 times, most recently from f67eef7 to c6e8896 Compare February 3, 2021 03:55
@jonchang
Copy link
Contributor

jonchang commented Feb 3, 2021

Pretty busy today but trying to help move this along. This currently fails if the bottle block only has a single, old-style bottle.

bottle node (single bottle)

(block
  (send nil :bottle)
  (args)
  (send nil :sha256
    (hash
      (pair
        (str "66e21f1df0e11d0aa6f810f821e8981247e9285d39e6579bd7376ebfc6482a58")
        (sym :x86_64_linux)))))
An error occurred while FormulaAudit/BottleOrder cop was inspecting /usr/local/Homebrew/Library/Taps/homebrew/homebrew-linux/Formula/libfuse.rb:1:0.
undefined method `method_name' for #<RuboCop::AST::HashNode:0x00007fbab4a34668>
Did you mean?  method_node
/usr/local/Homebrew/Library/Homebrew/rubocops/bottle.rb:145:in `block in audit_formula'
/usr/local/Homebrew/Library/Homebrew/rubocops/bottle.rb:144:in `each'
/usr/local/Homebrew/Library/Homebrew/rubocops/bottle.rb:144:in `audit_formula'
/usr/local/Homebrew/Library/Homebrew/rubocops/extend/formula.rb:39:in `on_class'

bottle node (multiple bottles)

(block
  (send nil :bottle)
  (args)
  (begin
    (send nil :sha256
      (hash
        (pair
          (str "66e21f1df0e11d0aa6f810f821e8981247e9285d39e6579bd7376ebfc6482a58")
          (sym :x86_64_linux))))
    (send nil :sha256
      (hash
        (pair
          (str "66e21f1df0e11d0aa6f810f821e8981247e9285d39e6579bd7376ebfc6482a58")
          (sym :fake_bottle))))))

@Rylan12 Rylan12 force-pushed the old-bottle-rubocop branch from c6e8896 to fd65ada Compare February 3, 2021 04:41
@Rylan12
Copy link
Member

Rylan12 commented Feb 3, 2021

Thanks. Should be fixed now.

@Rylan12
Copy link
Member

Rylan12 commented Feb 3, 2021

Here's the current status of this PR:

  1. brew style now requires the new bottle syntax. brew style --fix will auto-correct existing bottle blocks to the new syntax.
    • The new cellar: :any, big_sur: "aaaaaaaa" style is required
    • The bottle tags and digests are properly aligned
    • The bottle order is set such that arm bottles appear together and first
  2. brew bottle now writes bottle blocks in the new format.
  3. The bottle documentation is updated to reflect the new bottle syntax.

Currently, this is failing because homebrew/core and linuxbrew/core haven't been updated to use the new syntax. I believe that we should be able to change those over to the new syntax without breakage, but there will be a period of time where new bottles being added are still in an outdated style. It might be useful to either make this style rule a string rule (temporarily) until all formulae have been migrated. Or, the brew bottle changes can be pulled out and merged separately.

Running brew style --fix homebrew/core should handle changing all of these. For now, I'll plan on opening a PR in homebrew/core that makes this change (obviously CI-syntax-only). This should also help to discover any other rubocop errors. We should make sure everyone's on the same page before merging, though, as this will change every formula (that has a bottle) and will mean massive linux merges. I'm definitely open to suggestions for how to make this work in the best way.

@jonchang
Copy link
Contributor

jonchang commented Feb 3, 2021

I believe that we should be able to change those over to the new syntax without breakage, but there will be a period of time where new bottles being added are still in an outdated style.

Why would this be the case? I thought once this pull request is merged all newly written bottle blocks should be in the new format.

We should make sure everyone's on the same page before merging, though, as this will change every formula (that has a bottle) and will mean massive linux merges.

The Linux merge shouldn't be too much of an issue since we have various merge conflict scripts that can be adapted to handle this.

@Rylan12
Copy link
Member

Rylan12 commented Feb 3, 2021

I believe that we should be able to change those over to the new syntax without breakage, but there will be a period of time where new bottles being added are still in an outdated style.

Why would this be the case? I thought once this pull request is merged all newly written bottle blocks should be in the new format.

Ah, you're right. I forgot that the changes are applied during the merge not the test (i.e. brew bottle --write not brew bottle --json). However, we should probably still aim to merge the homebrew/core PR and this PR in between the hourly merges to be safe.

We should make sure everyone's on the same page before merging, though, as this will change every formula (that has a bottle) and will mean massive linux merges.

The Linux merge shouldn't be too much of an issue since we have various merge conflict scripts that can be adapted to handle this.

👍 That's good to hear

@Rylan12
Copy link
Member

Rylan12 commented Feb 3, 2021

Homebrew/core PR opened: Homebrew/homebrew-core#70283

Assuming this is deemed ready tomorrow, @jonchang and I will coordinate merging in homebrew/core, linuxbrew/core, and here to minimize the time for potential mixing of old and new bottle syntax.

The good thing is that since this new syntax is fully supported already, a mistake in the merge procedure should only cause CI breakage temporarily and should not negatively affect users.

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, thanks again all!

SeekingMeaning and others added 3 commits February 3, 2021 20:46
Co-authored-by: Rylan Polster <rslpolster@gmail.com>
Co-authored-by: Seeker <meaningseeking@protonmail.com>
@Rylan12 Rylan12 force-pushed the old-bottle-rubocop branch from fd65ada to 6e3011a Compare February 4, 2021 02:31
@Rylan12 Rylan12 merged commit 020c532 into Homebrew:master Feb 4, 2021
@Rylan12
Copy link
Member

Rylan12 commented Feb 4, 2021

Thanks, everyone!

lithammer added a commit to lithammer/homebrew-deadsnakes that referenced this pull request Feb 4, 2021
    brew style --fix ./Formula/*

Homebrew/brew#10453
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 7, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in progress Maintainers are working on this outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants