chromium: remove ofborg maintainer ping workaround, use CODEOWNERS#362543
Merged
Aleksanaa merged 1 commit intoNixOS:masterfrom Dec 7, 2024
Merged
chromium: remove ofborg maintainer ping workaround, use CODEOWNERS#362543Aleksanaa merged 1 commit intoNixOS:masterfrom
Aleksanaa merged 1 commit intoNixOS:masterfrom
Conversation
The workaround to have ofborg ping chromium and ungoogled-chromium maintainers when a change was only made to the upstream-info relied on string context. That string context was provided by the upstream-info being a nix file, not a json file, and then holding on to that string context using awkward attribute merges. It was intended as a quick fix until the handling of this would improve in ofborg itself and worked great. That was until very recently when we switched from the chromium release tarball to git source fetching in 8dd2f1a. Part of that change included going back from upstream-info.nix to upstream-info.json and with that losing the string context and the base on which this workaround used to work. But this is fine. A lot has happened in the meantime. CODEOWNERS was reimplemented and no longer requires every user listed in it to have write permissions to the repository (commit bit). Meaning we can accept that ofborg pings no longer work and instead rely on CODEOWNERS exclusively. It should, however, be noted that CODEOWNERS provide less granularity than ofborg, meaning we can no longer differentiate between ungoogled-chromium and chromium or even chromedriver. Previously, implementing the workaround that is now essentially reverted: 68c5979
github-actions
bot
added
the
6.topic: continuous integration
philiptaron
approved these changes
Dec 6, 2024
Contributor
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
Thanks for the thorough explanation in the PR, @emilylange. Sounds like ci/OWNERS will work great.
Contributor
|
I'm not sure the backport to 24.05 is a good idea. Do we even have |
github-actions
bot
added
10.rebuild-darwin: 0
networkException
approved these changes
Dec 6, 2024
Member
networkException
left a comment
networkException
left a comment
There was a problem hiding this comment.
Thanks for adding me
Member
We generally backport all commits relating to chromium to be able to remedy security issues and avoid conflicts |
Contributor
|
Successfully created backport PR for |
Merged
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The workaround to have ofborg ping
chromiumandungoogled-chromiummaintainers when a change was only made to the upstream-info relied on string context.That string context was provided by the upstream-info being a nix file, not a json file, and then holding on to that string context using awkward attribute merges.
It was intended as a quick fix until the handling of this would improve in ofborg itself and worked great.
That was until very recently when we switched from the chromium release tarball to git source fetching in #357371 (8dd2f1a).
Part of that change included going back from
upstream-info.nixtoupstream-info.jsonand with that losing the string context and the base on which this workaround used to work.But this is fine. A lot has happened in the meantime.
CODEOWNERS was reimplemented and no longer requires every user listed in it to have write permissions to the repository (commit bit).
Meaning we can accept that ofborg pings no longer work and instead rely on CODEOWNERS exclusively.
It should, however, be noted that CODEOWNERS provide less granularity than ofborg, meaning we can no longer differentiate between
ungoogled-chromiumandchromiumor evenchromedriver.Previously, implementing the workaround that is now essentially reverted: #245762 (68c5979)
This is a no-op.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)