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

project: move to nixpkgs Rust infrastructure #147

Merged
merged 2 commits into from
Jun 12, 2023

Conversation

RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Apr 14, 2023

This builds the stub and tool using rustPlatform.buildRustPackage which features a stable Rust compiler, recent enough to support UEFI targets.

In the future, it will rely on properly defined targets for UEFI in nixpkgs. : we won't need it.

For now, this PR is broken and depends on various changes in nixpkgs upstream for proper "cross compilation" support for UEFI targets, see NixOS/nixpkgs#226145 for example.

Closes #98.

nix/packages/stub.nix Outdated Show resolved Hide resolved
nix/packages/stub.nix Outdated Show resolved Hide resolved
@RaitoBezarius
Copy link
Member Author

RaitoBezarius commented May 27, 2023

This is ready for the initial review phase, final todolist:

  • bring back rustfmt validation: @nikstur can you help me here?
  • remove x86_64 hardcoding

@RaitoBezarius
Copy link
Member Author

Once we merge this, we can probably start upstreaming stable versions of Lanzaboote into nixpkgs.

Copy link
Member

@nikstur nikstur left a comment

Choose a reason for hiding this comment

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

Looks good, would really love to keep cargp fmt as a buildable derivation for nix flake check though.

nix/packages/tool.nix Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
nix/packages/stub.nix Outdated Show resolved Hide resolved
@RaitoBezarius RaitoBezarius force-pushed the nixpkgs-infrastructure branch 3 times, most recently from 5ddb719 to 75b14be Compare May 28, 2023 18:13
nix/packages/stub.nix Outdated Show resolved Hide resolved
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.

I'd use overrideAttrs for clippy and rustfmt rather than flags and merging in the main derivation expression. That way they could even live in separate files if you wanted.

nix/packages/stub.nix Outdated Show resolved Hide resolved
description = "Lanzaboote UEFI stub for SecureBoot enablement on NixOS systems";
homepage = "https://github.com/nix-community/lanzaboote";
license = licenses.mit;
platforms = [ "x86_64-windows" "aarch64-windows" "i686-windows" ];
Copy link
Member

Choose a reason for hiding this comment

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

Did we actually get building for aarch64 to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

error: Package ‘compiler-rt-14.0.6’ in /nix/store/wr7rzhqpb71986npaa2fvbjg302yb3r7-source/pkgs/development/compilers/llvm/14/compiler-rt/default.nix:128 is not available on the requested hostPlatform:

but passing the classical "unsupported systems ok" flag yields you

❯ llvm-objdump --section-headers /nix/store/xfjgpsn98yw3i0bc071zdk4606ij10bg-lanzastub-aarch64-windows-0.3.0/bin/lanzaboote_stub.efi 

/nix/store/xfjgpsn98yw3i0bc071zdk4606ij10bg-lanzastub-aarch64-windows-0.3.0/bin/lanzaboote_stub.efi:	file format coff-arm64

Sections:
Idx Name          Size     VMA              Type
  0 .text         00014770 0000000140001000 TEXT
  1 .rdata        000047f0 0000000140016000 DATA
  2 .data         00000070 000000014001b000 DATA
  3 .reloc        000003a8 000000014001c000 DATA

on a native aarch64 machine

Copy link
Member

@alyssais alyssais May 29, 2023

Choose a reason for hiding this comment

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

Yes, it wasn't a question whether it would build, but whether it would boot.

Remember the discussion about how aarch64 UEFI is not the same as aarch64 Windows that caused the argument over triples in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, testing until the end requires some support in systemd which is only on the main branch at the moment.

nix/packages/stub.nix Outdated Show resolved Hide resolved
nix/packages/stub.nix Outdated Show resolved Hide resolved
nix/packages/tool.nix Outdated Show resolved Hide resolved
@RaitoBezarius RaitoBezarius force-pushed the nixpkgs-infrastructure branch 3 times, most recently from eed1ee8 to f95a1fc Compare June 8, 2023 15:55
@RaitoBezarius
Copy link
Member Author

@nikstur can you make it so that the project compiles with all the features? right now we have mutually exclusive features which prevent me from doing cargo clippy --all-features :-[

@nikstur
Copy link
Member

nikstur commented Jun 10, 2023

@nikstur can you make it so that the project compiles with all the features? right now we have mutually exclusive features which prevent me from doing cargo clippy --all-features :-[

This is not possible in the current setup because the features are semantically exclusive. If we want to compile with all feature we would need to creare two separate binaries.

@RaitoBezarius
Copy link
Member Author

RaitoBezarius commented Jun 10, 2023 via email

This builds the stub and tool using `rustPlatform.buildRustPackage`
which features a stable Rust compiler, recent enough to support UEFI
targets.

In the future, it will rely on properly defined targets for UEFI in
nixpkgs.
…n transformer

Instead of patching the derivation in-place via flags, we just have
a higher order function that takes the Rust package derivation and override it
into a Rustfmt / Clippy oriented derivation: it turns off checks and adds its
required dependencies.
Copy link
Member

@nikstur nikstur left a comment

Choose a reason for hiding this comment

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

There are a few opportunities to minimize the diff but this is good to go. Thanks for the effort!

@RaitoBezarius RaitoBezarius merged commit 59e3ebb into master Jun 12, 2023
Comment on lines +66 to +67
config = "${pkgs.hostPlatform.qemuArch}-windows";
rustc.config = "${pkgs.hostPlatform.qemuArch}-unknown-uefi";
Copy link
Member

@SuperSandro2000 SuperSandro2000 Jun 13, 2023

Choose a reason for hiding this comment

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

Suggested change
config = "${pkgs.hostPlatform.qemuArch}-windows";
rustc.config = "${pkgs.hostPlatform.qemuArch}-unknown-uefi";
config = "${pkgs.stdenv.hostPlatform.qemuArch}-windows";
rustc.config = "${pkgs.stdenv.hostPlatform.qemuArch}-unknown-uefi";
       error: attribute 'hostPlatform' missing

       at /nix/store/k7h8k8whqw64rmsg6yhkmmdc82lkczc6-source/flake.nix:66:27:

           65|               # linuxArch is wrong here, it will yield arm64 instead of aarch64.
           66|               config = "${pkgs.hostPlatform.qemuArch}-windows";
             |                           ^
           67|               rustc.config = "${pkgs.hostPlatform.qemuArch}-unknown-uefi";
       Did you mean rustPlatform?

Going to test this and create a PR.

Copy link
Member

Choose a reason for hiding this comment

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

see #198

@nikstur nikstur deleted the nixpkgs-infrastructure branch July 20, 2023 22:03
nikstur added a commit that referenced this pull request Aug 12, 2023
…ture"

This reverts commit 59e3ebb, reversing
changes made to 9f97a90.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't use crane for production
4 participants