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

dispatch-build-bottle: support self-hosted Linux and unified workflow #11600

Merged
merged 1 commit into from Jun 25, 2021

Conversation

dawidd6
Copy link
Member

@dawidd6 dawidd6 commented Jun 25, 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?

This is going to be needed soon, when I merge dispatch workflows into one.

Companion PR: Homebrew/homebrew-core#80015

@BrewTestBot
Copy link
Member

Review period will end on 2021-06-28 at 17:41:55 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jun 25, 2021
@dawidd6 dawidd6 force-pushed the dispatch-linux-unify branch 2 times, most recently from 3cf8d76 to 63edd56 Compare June 25, 2021 18:02
@dawidd6
Copy link
Member Author

dawidd6 commented Jun 25, 2021

Weird typecheck errors strike again, can I get some help?

@dawidd6 dawidd6 added the critical Critical change which should be shipped as soon as possible. label Jun 25, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jun 25, 2021
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

BrewTestBot
BrewTestBot previously approved these changes Jun 25, 2021
@dawidd6
Copy link
Member Author

dawidd6 commented Jun 25, 2021

brew typecheck fails:

dev-cmd/dispatch-build-bottle.rb:86: Method `wheezy?` does not exist on `Homebrew::CLI::Args` https://srb.help/7003
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                                     ^^^^^^^^^^^^
  Got Homebrew::CLI::Args originating from:
    dev-cmd/dispatch-build-bottle.rb:41:
    41 |    args = dispatch_build_bottle_args.parse
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  Autocorrect: Use `-a` to autocorrect
    dev-cmd/dispatch-build-bottle.rb:86: Replace with `greedy?`
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                                          ^^^^^^^

dev-cmd/dispatch-build-bottle.rb:86: Method `wheezy?` does not exist on `Homebrew::CLI::Args` https://srb.help/7003
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                ^^^^^^^^^^^^
  Got Homebrew::CLI::Args originating from:
    dev-cmd/dispatch-build-bottle.rb:41:
    41 |    args = dispatch_build_bottle_args.parse
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  Autocorrect: Use `-a` to autocorrect
    dev-cmd/dispatch-build-bottle.rb:86: Replace with `greedy?`
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                     ^^^^^^^

dev-cmd/dispatch-build-bottle.rb:86: Method `wheezy?` does not exist on `Homebrew::CLI::Args` https://srb.help/7003
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                                     ^^^^^^^^^^^^
  Got Homebrew::CLI::Args originating from:
    dev-cmd/dispatch-build-bottle.rb:41:
    41 |    args = dispatch_build_bottle_args.parse
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  Autocorrect: Use `-a` to autocorrect
    dev-cmd/dispatch-build-bottle.rb:86: Replace with `greedy?`
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                                          ^^^^^^^

dev-cmd/dispatch-build-bottle.rb:86: Method `wheezy?` does not exist on `Homebrew::CLI::Args` https://srb.help/7003
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                ^^^^^^^^^^^^
  Got Homebrew::CLI::Args originating from:
    dev-cmd/dispatch-build-bottle.rb:41:
    41 |    args = dispatch_build_bottle_args.parse
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  Autocorrect: Use `-a` to autocorrect
    dev-cmd/dispatch-build-bottle.rb:86: Replace with `greedy?`
    86 |      inputs[:wheezy] = args.wheezy?.to_s if args.wheezy?
                                     ^^^^^^^
Errors: 4
Check https://docs.brew.sh/Typechecking for more information on how to resolve these errors.

@samford
Copy link
Member

samford commented Jun 25, 2021

You'll need to add a type definition for wheezy? to cli/args/args.rbi with the other returns(T::Boolean) methods. Basically, Sorbet can't automatically understand methods that are generated from CLI switches, so we have to manually tell it about them in an rbi file.

BrewTestBot
BrewTestBot previously approved these changes Jun 25, 2021
@dawidd6
Copy link
Member Author

dawidd6 commented Jun 25, 2021

Thanks. Got help on Slack too.

@dawidd6 dawidd6 marked this pull request as ready for review June 25, 2021 19:34
@dawidd6 dawidd6 enabled auto-merge June 25, 2021 19:36
@dawidd6 dawidd6 merged commit 71eb2fe into Homebrew:master Jun 25, 2021
@dawidd6 dawidd6 deleted the dispatch-linux-unify branch June 25, 2021 20:01
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 good, nice work!

Comment on lines +32 to +33
switch "--wheezy",
description: "Use Debian Wheezy container for building the bottle on Linux."
Copy link
Member

Choose a reason for hiding this comment

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

How about --linux-wheezy here?

Copy link
Member

Choose a reason for hiding this comment

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

Or even linux-debian-wheezy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't the description enough :)?

Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit weird to me not not reference it in this flag but have it as --linux-* in others.

@github-actions github-actions bot added the outdated PR was locked due to age label Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 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