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

style: fix on_macos/on_linux resource block checks #10118

Merged
merged 3 commits into from Dec 24, 2020

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 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 fixes a brew style bug for the on_macos and on_linux checks with resource blocks.

Previously, the following would cause an error that broke brew style (this example is from Homebrew/homebrew-core#67535):

resource "cargobootstrap" do
  on_macos do
    if Hardware::CPU.arch == :arm64
      url "https://static.rust-lang.org/dist/2020-12-23/cargo-beta-aarch64-apple-darwin.tar.gz"
      sha256 "efbc0e72533d4ca7def9a985feef4b3e43d24f1f6792815bdba9125af1f8ecdf"
    else
      url "https://static.rust-lang.org/dist/2020-11-19/cargo-1.48.0-x86_64-apple-darwin.tar.gz"
      sha256 "ce00d796cf5a9ac8d88d9df94c408e5d7ccd3541932a829eae833cc8e57efb15"
    end
  end
end

Now, this will trigger the style issue and give the following message:

only a url and a sha256 (in the right order) are allowed in a `on_macos` block within a resource block.

Edit: this is now allowed. The check will still verify that only url and sha256 or url, version, and sha256 methods are used in on_macos/on_linux blocks in resource blocks (even if they are nested in if statements)

@Rylan12 Rylan12 added the critical Critical change which should be shipped as soon as possible. label Dec 23, 2020
@BrewTestBot
Copy link
Member

BrewTestBot commented Dec 23, 2020

Review period ended.

@dtrodrigues
Copy link
Member

The crystal formula has a version section within its on_macos block, not just url and sha256:

Library/Taps/homebrew/homebrew-core/Formula/crystal.rb:62:3: C: FormulaAudit/ComponentsOrder: only a url and a sha256 (in the right order) are allowed in a on_macos block within a resource block.
  resource "boot" do ...
  ^^^^^^^^^^^^^^^^^^

5437 files inspected, 1 offense detected, 1 offense auto-correctable

Copy link
Member

@dtrodrigues dtrodrigues left a comment

Choose a reason for hiding this comment

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

Changes look good once CI passes on all of homebrew/core.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 23, 2020

Why do resource blocks need a version spec? Isn't it just based on the URL?

If it's needed, I think we should allow both:

resource "..." do
  on_macos do
    url "..."
    sha256 "..."
  end
end

and

resource "..." do
  on_macos do
    url "..."
    version "..."
    sha256 "..."
  end
end

Co-Authored-By: Rylan Polster <rslpolster@gmail.com>
Copy link
Contributor

@SeekingMeaning SeekingMeaning left a comment

Choose a reason for hiding this comment

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

I wrote some of this code so my review might be biased 😝

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 24, 2020

I wrote some of this code so my review might be biased 😝

Well, you can approve my code and I'll approve your code 😉 ✅

I'll let this sit for a tad longer in case some other maintainers have opinions. Since it's blocking a core PR, though, I'll merge it later today.

end
RUBY
end

it "the content of the on_linux block is wrong in a resource block" do
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I noticed was that a lot of the tests in the tests/rubocops directory don't have very good descriptions for it and context blocks. (I don't blame you @Rylan12 in this particular PR since the surrounding descriptions here aren't very... good)

For example, if the test surrounding this comment fails, here's what brew tests spits out:

Failures:

  1) RuboCop::Cop::FormulaAudit::ComponentsOrder resource the content of the on_linux block is wrong in a resource block

In contrast, here's what is outputted when this test in test/utils/curl_spec fails:

Failures:

  1) Utils::Curl curl_args returns --disable as the first argument when HOMEBREW_CURLRC is not set

This is an issue that probably should be addressed in one or more future PRs (by willing volunteers, of course)

Copy link
Member Author

@Rylan12 Rylan12 Dec 24, 2020

Choose a reason for hiding this comment

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

Yeah, I've certainly noticed this as well. I went ahead and fixed up just this section. Later, I can start going through and fixing up other sections as well.

@Rylan12 Rylan12 merged commit e6e76a4 into Homebrew:master Dec 24, 2020
@Rylan12 Rylan12 deleted the fix-style-on-os-blocks branch December 24, 2020 03:34
@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
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants