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: fix cross compilation #299031

Merged
merged 3 commits into from
Apr 16, 2024
Merged

chromium: fix cross compilation #299031

merged 3 commits into from
Apr 16, 2024

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Mar 25, 2024

Description of changes

  • Tested native compilation of chromium with native compilation on x86_64-linux and cross-compilation to aarch64-linux
  • Before this change we injected cross-compiled libraries into depsDepsBuild inputs.
    This happens because we override the libpng and libopus package mess
    with splicing.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

@Mic92 Mic92 mentioned this pull request Mar 25, 2024
13 tasks
@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Mar 25, 2024
@ofborg ofborg bot requested a review from edolstra March 25, 2024 23:17
@Mic92 Mic92 force-pushed the chromium-fix-2 branch 2 times, most recently from 4962810 to 523b005 Compare March 26, 2024 08:15
@Mic92 Mic92 marked this pull request as ready for review March 26, 2024 12:16
@Mic92 Mic92 requested a review from emilylange as a code owner March 26, 2024 12:16
@emilylange
Copy link
Member

Can you please share details about this?

Your PR description is empty, and your commit message only reads chromium: fix cross-compilation.

Cross-compilation puts lots of additional strain on chromium, a derivation that is already in a rough shape and heavily struggling to keep up at all.

How did you test your change?
Which architectures did you test?
Did you test electron, which depends on this chromium derivation, at all?

@Mic92
Copy link
Member Author

Mic92 commented Mar 27, 2024

Can you please share details about this?

Your PR description is empty, and your commit message only reads chromium: fix cross-compilation.

Cross-compilation puts lots of additional strain on chromium, a derivation that is already in a rough shape and heavily struggling to keep up at all.

How did you test your change? Which architectures did you test? Did you test electron, which depends on this chromium derivation, at all?

Updated. Let me know if you need more information. I currently didn't test any other derivations because it seems unlikely to me that adding depsDepsBuild inputs will change anything about them. Let me know if you want to test something in particular, I have enough build resources for that.

@avnik
Copy link
Contributor

avnik commented Mar 28, 2024

@emilylange this should fix #292346 and #292364

avnik added a commit to avnik/ghaf that referenced this pull request Mar 28, 2024
According NixOS/nixpkgs#299031
and https://github.com/NixOS/nixpkgs/compare/master...Mic92:chromium-fix?expand=1
(actually a overlayed changes from latter one)

Is also contain flag, producing excessive debug logs, in case of future problems
avnik added a commit to avnik/ghaf that referenced this pull request Mar 28, 2024
According NixOS/nixpkgs#299031
and https://github.com/NixOS/nixpkgs/compare/master...Mic92:chromium-fix?expand=1
(actually a overlayed changes from latter one)

Is also contain flag, producing excessive debug logs, in case of future problems
avnik added a commit to avnik/ghaf that referenced this pull request Mar 28, 2024
According NixOS/nixpkgs#299031
and https://github.com/NixOS/nixpkgs/compare/master...Mic92:chromium-fix?expand=1
(actually a overlayed changes from latter one)

Is also contain flag, producing excessive debug logs, in case of future problems
avnik added a commit to avnik/ghaf that referenced this pull request Apr 2, 2024
According NixOS/nixpkgs#299031
and https://github.com/NixOS/nixpkgs/compare/master...Mic92:chromium-fix?expand=1
(actually a overlayed changes from latter one)

Is also contain flag, producing excessive debug logs, in case of future problems

Signed-off-by: Alexander Nikolaev <alexander.nikolaev@unikie.com>
@emilylange
Copy link
Member

I don't see how c4492425adabbe8e75515753590baf1bf51766ca is needed since #298515 has been merged to staging and will probably land in master the coming weeks.

Additionally, from what I know, xdg-utils not cross-compiling does not prevent a chromium/electron cross compilation build from succeeding.

Amjoseph, who is responsible for most of the cross-compilation work in chromium, added xdg-utils.meta.broken conditionals for this.

I am fine with the cross-compilation fix for chromium (and by proxy electron) itself.
But also note that I won't notice any issues with it if there are some, because I don't know much about cross-compilation.

Would you mind dropping your xdg-utils fix from this PR (because #298515 has been merged instead of this) and rebase and re-target master?

Thanks.

@emilylange emilylange marked this pull request as draft April 4, 2024 01:11
brianmcgillion pushed a commit to tiiuae/ghaf that referenced this pull request Apr 4, 2024
According NixOS/nixpkgs#299031
and https://github.com/NixOS/nixpkgs/compare/master...Mic92:chromium-fix?expand=1
(actually a overlayed changes from latter one)

Is also contain flag, producing excessive debug logs, in case of future problems

Signed-off-by: Alexander Nikolaev <alexander.nikolaev@unikie.com>
@Mic92
Copy link
Member Author

Mic92 commented Apr 4, 2024

xdg-utils was used in the chromium shell wrapper. It's not used in the chromium build itself. I can check if #298515 fixes the issue.

@Mic92
Copy link
Member Author

Mic92 commented Apr 4, 2024

There should be enough engineering capacity in the ghaf project to also ensure in future that cross-compilation will continue to work for chromium.

@emilylange
Copy link
Member

xdg-utils was used in the chromium shell wrapper. It's not used in the chromium build itself.

Ah, so my information was outdated. xdg-utils cross-compilation is no longer marked as broken since #246954. I did not notice that. See #225112 and #225114 for context.

That's good to know.

I can check if #298515 fixes the issue.

Would be cool if you could report back when you find some time and compute for this :)

There should be enough engineering capacity in the ghaf project to also ensure in future that cross-compilation will continue to work for chromium.

What is the /graf project/ you are referring to?

@Mic92
Copy link
Member Author

Mic92 commented Apr 5, 2024

Ghaf is something like QubesOS but using NixOS and KVM-based virtualisation: https://tiiuae.github.io/ghaf/

@Mic92 Mic92 marked this pull request as ready for review April 8, 2024 12:40
@Mic92
Copy link
Member Author

Mic92 commented Apr 8, 2024

My xdg-utils fix is no longer needed. Still waiting for the chromium build to finish.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Apr 8, 2024
@Mic92
Copy link
Member Author

Mic92 commented Apr 8, 2024

However now I found a mesa regression that is new...

@Mic92
Copy link
Member Author

Mic92 commented Apr 9, 2024

Chromium builds again. Finished from my side.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Apr 12, 2024
@@ -123,6 +123,12 @@ buildPythonPackage rec {
"test_partialfunction"
];

disabledTestPaths = [
Copy link
Member

Choose a reason for hiding this comment

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

Also, is there a reason why you used disabledTestPaths instead of disabledTests?

Copy link
Member Author

@Mic92 Mic92 Apr 12, 2024

Choose a reason for hiding this comment

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

There are multiple tests that all break occasionally, listing them manually will possible add regressions in the future because they don't seem to always break.

pkgs/applications/networking/browsers/chromium/common.nix Outdated Show resolved Hide resolved
pkgs/by-name/me/meson/package.nix Outdated Show resolved Hide resolved
- Tested with native compilation on x86_64-linux and cross-compilation to aarch64-linux
- Before this change we injected cross-compiled libraries into depsDepsBuild inputs.
  This happens because we override the libpng and libopus package mess
  with splicing.
Copy link
Member

@emilylange emilylange left a comment

Choose a reason for hiding this comment

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

Thanks

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Apr 12, 2024
@Mic92 Mic92 merged commit 4590d4d into NixOS:staging Apr 16, 2024
25 checks passed
@Mic92 Mic92 deleted the chromium-fix-2 branch April 16, 2024 11:11
brianmcgillion added a commit to brianmcgillion/ghaf that referenced this pull request May 31, 2024
fixed in upstream
NixOS/nixpkgs#299031
NixOS/nixpkgs#308196

Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
brianmcgillion added a commit to tiiuae/ghaf that referenced this pull request Jun 3, 2024
fixed in upstream
NixOS/nixpkgs#299031
NixOS/nixpkgs#308196

Signed-off-by: Brian McGillion <bmg.avoin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: python 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants