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.

Sign up for GitHub

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

Jump to bottom

just: fix cross-compilation #351032

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

just: fix cross-compilation #351032

wants to merge 2 commits into from
+40 −35

Conversation

xokdvium
Copy link
Contributor

@xokdvium xokdvium commented Oct 24, 2024 •
edited
Loading

Fixes #320569.

Get rid of calls to cargo run in a, which lead to cargo TARGET and CC
to be out of sync for cross-compilation.

Use emulator to build man pages, shell completions and source for the docs.

With this change I'm able to build pkgsCross.aarch64-multiplatform.just as well as pkgsCross.aarch64-multiplatform.pkgsStatic.just from x86_64-linux.

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.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

alyssais
alyssais reviewed Oct 24, 2024
View reviewed changes
pkgs/by-name/ju/just/package.nix Outdated
src
cargoHash
;
nativeBuildInputs = [ buildPackages.mdbook ];
Copy link
Member

Choose a reason for hiding this comment

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

Given this is a build dependency for a build dependency, it should come from pkgsBuildBuild. (But also if you just use "mdbook" splicing should do the right thing.)

alyssais
alyssais reviewed Oct 24, 2024
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

An easier and more portable approach would probably be to just copy the documentation from buildPackages.just when cross compiling.

@xokdvium xokdvium force-pushed the dev/just-fix-cross-compilation branch 2 times, most recently from 1c081f1 to 285d69c Compare October 24, 2024 22:19
@xokdvium
Copy link
Contributor Author

An easier and more portable approach would probably be to just copy the documentation from buildPackages.just when cross compiling.

Thanks for the tip! I wasn't sure how to best handle it in this case.
In fact the docs were building fine when retargeting cargo for the host system, but that seemed fragile to me. I implemented it how you suggested:

For native compilation we buid the docs if withDocs is set, if we are cross-compiling then they get copied from buildPackages.just.

It's a bit more work to compile just natively than only the docs, but a more general solution is preferable in this case.

@xokdvium
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review

x86_64-linux

✅ 97 packages built:
  • aba
  • catppuccin-cursors
  • catppuccin-cursors.frappeBlue
  • catppuccin-cursors.frappeDark
  • catppuccin-cursors.frappeFlamingo
  • catppuccin-cursors.frappeGreen
  • catppuccin-cursors.frappeLavender
  • catppuccin-cursors.frappeLight
  • catppuccin-cursors.frappeMaroon
  • catppuccin-cursors.frappeMauve
  • catppuccin-cursors.frappePeach
  • catppuccin-cursors.frappePink
  • catppuccin-cursors.frappeRed
  • catppuccin-cursors.frappeRosewater
  • catppuccin-cursors.frappeSapphire
  • catppuccin-cursors.frappeSky
  • catppuccin-cursors.frappeTeal
  • catppuccin-cursors.frappeYellow
  • catppuccin-cursors.latteBlue
  • catppuccin-cursors.latteDark
  • catppuccin-cursors.latteFlamingo
  • catppuccin-cursors.latteGreen
  • catppuccin-cursors.latteLavender
  • catppuccin-cursors.latteLight
  • catppuccin-cursors.latteMaroon
  • catppuccin-cursors.latteMauve
  • catppuccin-cursors.lattePeach
  • catppuccin-cursors.lattePink
  • catppuccin-cursors.latteRed
  • catppuccin-cursors.latteRosewater
  • catppuccin-cursors.latteSapphire
  • catppuccin-cursors.latteSky
  • catppuccin-cursors.latteTeal
  • catppuccin-cursors.latteYellow
  • catppuccin-cursors.macchiatoBlue
  • catppuccin-cursors.macchiatoDark
  • catppuccin-cursors.macchiatoFlamingo
  • catppuccin-cursors.macchiatoGreen
  • catppuccin-cursors.macchiatoLavender
  • catppuccin-cursors.macchiatoLight
  • catppuccin-cursors.macchiatoMaroon
  • catppuccin-cursors.macchiatoMauve
  • catppuccin-cursors.macchiatoPeach
  • catppuccin-cursors.macchiatoPink
  • catppuccin-cursors.macchiatoRed
  • catppuccin-cursors.macchiatoRosewater
  • catppuccin-cursors.macchiatoSapphire
  • catppuccin-cursors.macchiatoSky
  • catppuccin-cursors.macchiatoTeal
  • catppuccin-cursors.macchiatoYellow
  • catppuccin-cursors.mochaBlue
  • catppuccin-cursors.mochaDark
  • catppuccin-cursors.mochaFlamingo
  • catppuccin-cursors.mochaGreen
  • catppuccin-cursors.mochaLavender
  • catppuccin-cursors.mochaLight
  • catppuccin-cursors.mochaMaroon
  • catppuccin-cursors.mochaMauve
  • catppuccin-cursors.mochaPeach
  • catppuccin-cursors.mochaPink
  • catppuccin-cursors.mochaRed
  • catppuccin-cursors.mochaRosewater
  • catppuccin-cursors.mochaSapphire
  • catppuccin-cursors.mochaSky
  • catppuccin-cursors.mochaTeal
  • catppuccin-cursors.mochaYellow
  • catppuccin-sddm
  • celeste
  • cosmic-applets
  • cosmic-applibrary
  • cosmic-bg
  • cosmic-design-demo
  • cosmic-edit
  • cosmic-files
  • cosmic-greeter
  • cosmic-icons
  • cosmic-launcher
  • cosmic-notifications
  • cosmic-panel
  • cosmic-randr
  • cosmic-screenshot
  • cosmic-session
  • cosmic-settings
  • cosmic-store
  • cosmic-term
  • dogdns
  • dogdns.man
  • find-billy
  • forecast
  • just
  • just.doc
  • just.man
  • kabeljau
  • onagre
  • pop-launcher
  • ssh-openpgp-auth
  • sshd-openpgp-auth

@xokdvium
Copy link
Contributor Author

Also able to build pkgsCross.riscv64.just in addition to the dynamic/static builds for aarch64 from x86_64-linux

@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Oct 25, 2024
@ofborg ofborg bot requested review from xrelkd and 06kellyjac October 25, 2024 02:52
alyssais
alyssais reviewed Oct 25, 2024
View reviewed changes
pkgs/by-name/ju/just/package.nix
)
+ lib.optionalString (stdenv.hostPlatform.emulatorAvailable buildPackages) ''
${just} --man > ./just.1
installManPage ./just.1
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we copying this from buildPackages.just as well?

Copy link
Contributor Author

@xokdvium xokdvium Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've looked back on the solution with the copying and it doesn't really sit well with me.
I refactored generate-book to use emulation. As it turns out it's not a bottleneck at all and takes very little time even under emulation.
Sorry for the back and forth

Copy link
Member

Choose a reason for hiding this comment

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

Emulation should really only be used where it's a last resort, because we don't have emulators for every platform we might want to cross compile to.

Copy link
Member

Choose a reason for hiding this comment

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

We've discussed it a bit here #351032, but I don't have a clear opinion on this

Copy link
Member

Choose a reason for hiding this comment

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

Wrong link?

Copy link
Member

Choose a reason for hiding this comment

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

Oh #308283

@xokdvium xokdvium force-pushed the dev/just-fix-cross-compilation branch from 285d69c to 4986a17 Compare October 25, 2024 08:04
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 10, 2024
@xokdvium xokdvium marked this pull request as draft January 4, 2025 15:36
@xokdvium xokdvium force-pushed the dev/just-fix-cross-compilation branch from 4986a17 to 7e32534 Compare January 4, 2025 15:39
@xokdvium xokdvium marked this pull request as ready for review January 4, 2025 15:57
@xokdvium
Copy link
Contributor Author

xokdvium commented Jan 4, 2025

Rebased and updated. Since there still doesn't seem to be an agreement on how to handle cross-compilation for generating shell completions and documentation, I've stuck with the current consensus and skip those steps based on stdenv.buildPlatform.canExecute stdenv.hostPlatform.

@xokdvium
just: fix cross-compilation
596dfcb 
Get rid of calls to `cargo run` in a, which lead to cargo TARGET and CC
to be out of sync for cross-compilation.

Only build man pages, shell completions and source for the docs when
`stdenv.buildPlatform.canExecute stdenv.hostPlatform`, as this seems to be
the current consensus.
@xokdvium xokdvium force-pushed the dev/just-fix-cross-compilation branch from 7e32534 to 596dfcb Compare January 4, 2025 16:09
06kellyjac
06kellyjac approved these changes Jan 6, 2025
View reviewed changes
Copy link
Member

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

Non-blocking suggestion to help any future reviewers of the code.

I'm happy with this method or copying results from buildPackages. Emulation methods are fun but as discussed in the other issue require an excessively large closure and can be brittle.

pkgs/by-name/ju/just/package.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 6, 2025
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 6, 2025
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aleksanaa Aleksanaa Aleksanaa left review comments

@alyssais alyssais alyssais left review comments

@06kellyjac 06kellyjac 06kellyjac approved these changes

@xrelkd xrelkd Awaiting requested review from xrelkd

Assignees
No one assigned
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Build failure: just cross-compliation fails
5 participants
@xokdvium @alyssais @06kellyjac @Aleksanaa @wegank