-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
ci_matrix: Rework depends_on array macos handling #189969
Conversation
6c65826
to
84b4d33
Compare
Thanks @samford - I've incorporated into the PR over in Brew as well. I have done some testing with a couple of cases and it seems to work correctly for me. |
Thanks for testing this some more. It's morning over here, so I'm going to go ahead and merge this now that I can keep an eye on CI (and revert if needed). After this is merged, I'll open a draft PR that modifies a [random] existing cask to use a |
A good test is |
CodeQL complains about parts of the `DEPENDS_ON_MACOS_ARRAY_MEMBER` regex snippet: `This part of the regular expression may cause exponential backtracking on strings starting with 'depends_on macos: [:' and containing many repetitions of '!:'.` This error occurs in relation to the repeating `[^\\s",]` and trailing `\s` parts of this regex. I'm not very familiar with the issue CodeQL is flagging but this reworks handling of `depends_on macos:` arrays in such a way that CodeQL doesn't return any errors. In the process of addressing this, I updated the array member regex so it's also capable of matching strings with a leading comparator (e.g., `">= :big_sur"`) instead of only symbols like `:big_sur`. I also opted to simplify the case condition regex and inline the array member regex, as it may be easier to understand compared to having a separate regex constant.
84b4d33
to
1eab88f
Compare
Ah! Thanks for the heads up. For some reason I didn't catch any |
Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.
In the following questions
<cask>
is the token of the cask you're submitting.After making any changes to a cask, existing or new, verify:
brew audit --cask --online <cask>
is error-free.brew style --fix <cask>
reports no offenses.Additionally, if adding a new cask:
brew audit --cask --new <cask>
worked successfully.HOMEBREW_NO_INSTALL_FROM_API=1 brew install --cask <cask>
worked successfully.brew uninstall --cask <cask>
worked successfully.CodeQL complains about parts of the
DEPENDS_ON_MACOS_ARRAY_MEMBER
regex snippet:This part of the regular expression may cause exponential backtracking on strings starting with 'depends_on macos: [:' and containing many repetitions of '!:'.
This error occurs in relation to the repeating[^\\s",]
and trailing\s
parts of this regex.I'm not very familiar with the issue CodeQL is flagging but this reworks handling of
depends_on macos:
arrays in such a way that CodeQL doesn't return any errors. In the process of addressing this, I updated the array member regex so it's also capable of matching strings with a leading comparator (e.g.,">= :big_sur"
) instead of only symbols like:big_sur
. If we only ever need to match symbols, I'll update this to use the existing.flatten.map(&:to_sym).map
approach.I also opted to simplify the case condition regex and inline the array member regex, as it may be easier to understand compared to having a separate regex constant.
I'll mark this as ready for review in the morning but I've set it as a draft for now, as I didn't want to risk messing up CI while I'm asleep. I manually tested the logic but better safe than sorry.
Related to Homebrew/brew#18594