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

chromium: remove xdg-utils build dependency as it is wrapped later anyway #246038

Merged
merged 1 commit into from Mar 2, 2024

Conversation

Philipp-M
Copy link
Member

I don't think it's necessary to bundle/"hard-code" xdg-utils into the chromium source code, as all the binaries in there aren't hardcoded (I checked the relevant chromium source). Since xdg-utils is later added/wrapped to the PATH anyway here:

export PATH="\$PATH\''${PATH:+:}${xdg-utils}/bin"

I think we should probably minimize build dependencies as much as possible if it can be wrapped later to avoid cache invalidation.
I'm currently rebuilding the world (and especially chromium takes up a good chunk of the time) because of something like this:
https://github.com/Philipp-M/nixos-config/blob/28d77ea08aaccf4544de3d2903210cd6037e3ca2/configuration.nix#L23

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

@emilylange
Copy link
Member

There is some history to this from 2017-2019:

I think we should probably minimize build dependencies as much as possible if it can be wrapped later to avoid cache invalidation.

I agree. The chromium derivation, however, is in a very rough shape.
E.g. re2 is used as build dependency, but never actually used, since we default to the vendored version.
It will continue to take a long time to clean up the chromium derivation from all the past crimes (/hj).
Thanks for your work and contribution :)

I've tested the build manually. xdg-* related things seem to continue to work as expected.

I've seen this PR a while ago, but I unfortunately couldn't find any spare time back then.
Chromium simply hasn't enough maintainers in nixpkgs. Sorry for taking so long to respond, nonetheless.

There is a merge conflict in pkgs/applications/networking/browsers/chromium/common.nix now.
Can you solve this yourself or do you need any help?

PS @SuperSandro2000: Hope you don't mind, I made the link in your comment a permalink by replacing main with 121.0.6167.139 (the latest version currently present in our channels). So when this inevitable blows up again, or someone find this PR for any other reason, it points to the same code revision from when the comment was made. Chromium is a very fast moving code base 🫠

@Philipp-M
Copy link
Member Author

I've seen this PR a while ago, but I unfortunately couldn't find any spare time back then.
Chromium simply hasn't enough maintainers in nixpkgs. Sorry for taking so long to respond, nonetheless.

Well, sorry that I took now a little bit longer to respond :)

I have updated the PR

@emilylange
Copy link
Member

Started a final nixpkgs-review that will probably take a day to complete (2 chromiums, 4 electrons).

I intend to manually test one chromium and one electron-based application (probably element-desktop).

If everything goes well (I'd be surpised if something doesn't), then I will hit merge.

Thanks again :)

Do you want/need this to be backported to 23.11 as well?

@Philipp-M
Copy link
Member Author

Do you want/need this to be backported to 23.11 as well?

I don't think that's necessary. (At least for me)

@emilylange
Copy link
Member

Result of nixpkgs-review pr 246038 run on x86_64-linux 1

2 packages marked as broken and skipped:
  • archivebox
  • archivebox.dist
1 package failed to build:
  • redisinsight
73 packages built:
  • authy
  • aws-azure-login
  • bilibili
  • bitwarden-desktop
  • bitwarden-directory-connector
  • bruno
  • camunda-modeler
  • chromium
  • chromium.sandbox
  • deltachat-desktop
  • drawio
  • drawio-headless
  • electron (electron_28)
  • electron_26
  • electron_27
  • element-desktop
  • element-desktop-wayland
  • fast-cli
  • freetube
  • headset
  • heroic
  • heroic-unwrapped
  • itch
  • jitsi-meet-electron
  • logseq
  • mattermost-desktop
  • mermaid-cli
  • mermaid-filter
  • micropad
  • morgen
  • nix-tour
  • obsidian
  • open-stage-control
  • pandoc-drawio-filter
  • pandoc-drawio-filter.dist
  • percollate
  • playwright-test
  • pocket-casts
  • podman-desktop
  • pritunl-client
  • puppeteer-cli
  • python311Packages.mkdocs-drawio-exporter
  • python311Packages.mkdocs-drawio-exporter.dist
  • python311Packages.pytest-playwright
  • python311Packages.pytest-playwright.dist
  • python312Packages.mkdocs-drawio-exporter
  • python312Packages.mkdocs-drawio-exporter.dist
  • python312Packages.pytest-playwright
  • python312Packages.pytest-playwright.dist
  • r2modman
  • revolt-desktop
  • sharedown
  • sitespeed-io
  • standardnotes
  • stretchly
  • super-productivity
  • teams-for-linux
  • terra-station
  • thedesk
  • threema-desktop
  • uhk-agent
  • uhk-udev-rules
  • uivonim
  • ungoogled-chromium
  • ungoogled-chromium.sandbox
  • vesktop
  • vhs
  • vieb
  • wayback
  • webcord
  • webcord-vencord
  • webtorrent_desktop
  • youtube-music

# git describe --abbrev=40
24.05-pre-39203-g0267739e11e44bb791b747565758c6446b01eac7
# nix-build nixos/tests/chromium.nix -A stable -A ungoogled
/nix/store/6l1rl1mn51arb48zb1gd60hmzzi6bx4d-vm-test-run-chromium-stable
/nix/store/qmmr5vhg5v9vgwkxwlskjxpgc5hsq9y2-vm-test-run-chromium-ungoogled

I also manually tested element-desktop and chromium under wayland (sway).

LGTM

@emilylange emilylange merged commit 78dc461 into NixOS:master Mar 2, 2024
24 checks passed
@Philipp-M Philipp-M deleted the chromium-no-patched-xdg-utils branch March 2, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants