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

add determine-cask-runners command #18594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bevanjkay
Copy link
Member

  • 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 a draft of moving the commands that generate the matrixes for our homebrew-cask CI to a DevCmd in Brew.

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 so far!

@bevanjkay bevanjkay force-pushed the determine-cask-runners branch 2 times, most recently from eb4536d to 94318f4 Compare October 26, 2024 12:33
@samford
Copy link
Member

samford commented Oct 26, 2024

Testing this locally using the CodeQL CLI, the errors relate to the repeating [^\s",] and trailing \s parts of [^\s",]+)"?,?\s* in the DEPENDS_ON_MACOS_ARRAY_MEMBER regex snippet. I'm not very knowledgeable about the issue that CodeQL is referencing but using possessive quantifiers (++ and *+ respectively) seems to be one way to resolve the errors.

However, I tested the regex changes locally and it works for something like depends_on macos: [":big_sur", ":sonoma"] but doesn't match something like depends_on macos: [:big_sur, :sonoma]. We only use a string for something like >= :big_sur, so that's a problem.

Making the repeating parts non-greedy ([^\\s",]+?, \s*?) doesn't trigger the CodeQL errors but I don't know if that addresses the underlying issue or just sidesteps the errors. After tinkering with this some more, I ended up simplifying the case condition regex and expanding the array member regex to also handle strings with a comparator (it currently only handles symbols and strings with symbol text like ":big_sur"). [MacosRequirement can only handle an array of symbols currently but there may be something to be said for handling something like [">= :big_sur", "<= :sequoia"], as that's less verbose than explicitly specifying every macOS version in that range.]

I've created a PR in homebrew/cask first as a smoke test (Homebrew/homebrew-cask#189969) and, if nothing explodes, I'll push the changes here as well.

@bevanjkay bevanjkay force-pushed the determine-cask-runners branch 6 times, most recently from ef951d1 to 9784015 Compare October 28, 2024 07:19
@bevanjkay bevanjkay marked this pull request as ready for review October 28, 2024 08:29
@bevanjkay
Copy link
Member Author

The tests aren't exhaustive, I'm not exactly how much coverage we are looking for here, as any major concerns will be revealed in CI which we can't quite replicate in the test suite.

@bevanjkay bevanjkay force-pushed the determine-cask-runners branch 2 times, most recently from 5a7a9e0 to faf7830 Compare October 28, 2024 12:05
end

sig { params(cask_content: String).returns(T::Hash[T::Hash[Symbol, Symbol], Float]) }
def filter_runners(cask_content)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't need to be in this PR, but this should probably have some unit tests.

@bevanjkay bevanjkay force-pushed the determine-cask-runners branch 2 times, most recently from e00ebd9 to ff05d39 Compare November 11, 2024 12:08
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.

Thanks @bevanjkay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants