raito/lanzaboote

Fork 0

project: move to nixpkgs Rust infrastructure #147

Merged

RaitoBezarius merged 2 commits from nixpkgs-infrastructure into master

2023-06-12 08:40:17 +00:00

RaitoBezarius commented

2023-04-14 12:57:13 +00:00

(Migrated from github.com)

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 https://github.com/NixOS/nixpkgs/pull/226145 for example.~~

Closes #98.

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 https://github.com/NixOS/nixpkgs/pull/226145 for example.~ Closes #98.

blitz (Migrated from github.com) reviewed

2023-04-14 12:57:13 +00:00

Mic92 (Migrated from github.com) reviewed

2023-04-14 12:57:13 +00:00

RaitoBezarius (Migrated from github.com) reviewed

2023-04-14 12:57:48 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-04-14 12:57:48 +00:00

TODO: once the mentioned PR land, replace this by just using the proper example system.

RaitoBezarius (Migrated from github.com) reviewed

2023-04-14 12:58:06 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-04-14 12:58:06 +00:00

TODO: figure out what we want here.

RaitoBezarius commented

2023-05-27 21:20:22 +00:00

(Migrated from github.com)

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

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

This is ready for the initial review phase, final todolist: - [x] bring back `rustfmt` validation: @nikstur can you help me here? - [x] remove x86_64 hardcoding

RaitoBezarius commented

2023-05-27 21:22:19 +00:00

(Migrated from github.com)

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

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

🚀 1

nikstur (Migrated from github.com) approved these changes

2023-05-27 21:36:39 +00:00

nikstur (Migrated from github.com) left a comment

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

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

flake.nix

nikstur (Migrated from github.com) commented

2023-05-27 21:36:01 +00:00

The most cursed of all arguments....

nix/packages/tool.nix

nikstur (Migrated from github.com) commented

2023-05-27 21:30:35 +00:00

This fells really janky, but I guess there is not really a better way. Can you also add something like this for cargo fmt?

This fells really janky, but I guess there is not really a better way. Can you also add something like this for `cargo fmt`?

RaitoBezarius (Migrated from github.com) reviewed

2023-05-27 21:38:27 +00:00

nix/packages/tool.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-27 21:38:27 +00:00

actually I find it really neat, we don't need to build the project to run clippy or cargo fmt.
Of course, we can be more fancy and extract the drvs, etc and put a proper checkPhase for that, but that seems wasteful?

actually I find it really neat, we *don't need* to build the project to run clippy or cargo fmt. Of course, we can be more fancy and extract the drvs, etc and put a proper checkPhase for that, but that seems wasteful?

RaitoBezarius (Migrated from github.com) reviewed

2023-05-27 21:38:37 +00:00

flake.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-27 21:38:37 +00:00

I find it beautiful

nikstur (Migrated from github.com) reviewed

2023-05-27 21:43:41 +00:00

nix/packages/tool.nix

nikstur (Migrated from github.com) commented

2023-05-27 21:43:41 +00:00

Nono, please let's not make this more complicated than it needs to be. I kind of just wished that there was a flag you can just enable in buildRustPackage.

we don't need to build the project to run clippy

True, but running clippy is almost like building the project.

In any case, I think your solution is good for what we can do right here downstream, but I just wished there was better support for these things upstream (or I guess for development crane really kind of is just better).

Nono, please let's not make this more complicated than it needs to be. I kind of just wished that there was a flag you can just enable in `buildRustPackage`. > we _don't need_ to build the project to run clippy True, but running clippy is almost like building the project. In any case, I think your solution is good for what we can do right here downstream, but I just wished there was better support for these things upstream (or I guess for development `crane` really kind of is just better).

RaitoBezarius (Migrated from github.com) reviewed

2023-05-27 22:42:47 +00:00

nix/packages/tool.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-27 22:42:47 +00:00

Feel free to go upstream and propose stuff :P

alyssais (Migrated from github.com) reviewed

2023-05-28 08:41:52 +00:00

nix/packages/tool.nix

alyssais (Migrated from github.com) commented

2023-05-28 08:41:52 +00:00

pls no piling more stuff into buildRustPackage — it doesn't compose and makes Cargo special compared to other build systems for no reason. I'd be okay with a clippy setup hook though.

alyssais (Migrated from github.com) approved these changes

2023-05-28 08:45:45 +00:00

nix/packages/stub.nix

alyssais (Migrated from github.com) commented

2023-05-28 08:45:18 +00:00

Is it intentional that these extra attributes bypass buildRustPackage? (You need to wrap the attrset merge in parens if you don't want it to — ({ … } // { … }).

Is it intentional that these extra attributes bypass `buildRustPackage`? (You need to wrap the attrset merge in parens if you don't want it to — `({ … } // { … })`.

SuperSandro2000 (Migrated from github.com) reviewed

2023-05-28 13:16:32 +00:00

nix/packages/tool.nix

SuperSandro2000 (Migrated from github.com) commented

2023-05-28 13:16:32 +00:00

Why not have it in passthru.lint and build $NAME.lint?

RaitoBezarius (Migrated from github.com) reviewed

2023-05-28 14:49:02 +00:00

nix/packages/tool.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-28 14:49:02 +00:00

I cannot do this if I don't have any recursive reference to myself, which is not possible because I don't have finalAttrs, neither a proper callPackage reinjector AFAIK.

RaitoBezarius (Migrated from github.com) reviewed

2023-05-28 14:49:21 +00:00

nix/packages/tool.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-28 14:49:21 +00:00

@nikstur Added something for cargo fmt, let me know if this is not enough.

RaitoBezarius (Migrated from github.com) reviewed

2023-05-28 14:49:39 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-28 14:49:38 +00:00

This is on purpose because I don't need the buildRustPackage stuff

RaitoBezarius (Migrated from github.com) reviewed

2023-05-28 19:44:42 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-28 19:44:42 +00:00

@alyssais could you check again this? I find it ugly but I'm not sure I can avoid it.

alyssais (Migrated from github.com) reviewed

2023-05-28 20:25:10 +00:00

alyssais (Migrated from github.com) left a comment

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.

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

alyssais (Migrated from github.com) commented

2023-05-28 20:21:18 +00:00

wouldn't lib.cleanSource make more sense?

wouldn't `lib.cleanSource` make more sense?

alyssais (Migrated from github.com) commented

2023-05-28 20:22:40 +00:00

Did we actually get building for aarch64 to work?

alyssais (Migrated from github.com) commented

2023-05-28 20:24:35 +00:00

Shouldn't this be targetSpec?

RaitoBezarius (Migrated from github.com) reviewed

2023-05-28 22:59:04 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-28 22:59:04 +00:00

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

`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

RaitoBezarius (Migrated from github.com) reviewed

2023-05-28 23:14:39 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-28 23:14:39 +00:00

Right

RaitoBezarius (Migrated from github.com) reviewed

2023-05-28 23:14:39 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-28 23:14:39 +00:00

Hmm, I was not sure what target could take as an arg, but I guess so

alyssais (Migrated from github.com) reviewed

2023-05-29 09:04:08 +00:00

nix/packages/stub.nix

alyssais (Migrated from github.com) commented

2023-05-29 09:04:08 +00:00

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.

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.

RaitoBezarius (Migrated from github.com) reviewed

2023-05-29 10:21:23 +00:00

nix/packages/stub.nix

RaitoBezarius (Migrated from github.com) commented

2023-05-29 10:21:22 +00:00

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

nikstur (Migrated from github.com) reviewed

2023-05-29 12:00:35 +00:00

nix/packages/stub.nix

nikstur (Migrated from github.com) commented

2023-05-29 11:48:24 +00:00

  pname = "lanzaboote_stub";

```suggestion pname = "lanzaboote_stub"; ```

nix/packages/tool.nix

nikstur (Migrated from github.com) commented

2023-05-29 11:49:11 +00:00

  pname = "lanzaboote_tool";

```suggestion pname = "lanzaboote_tool"; ```

RaitoBezarius commented

2023-06-08 16:25:09 +00:00

(Migrated from github.com)

@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 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 commented

2023-06-10 12:30:52 +00:00

(Migrated from github.com)

@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.

> @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 commented

2023-06-10 12:59:35 +00:00

(Migrated from github.com)

I see, then, no problem, I will find a way :-)

Le sam. 10 juin 2023 à 14:31, nikstur @.***> a écrit :

@nikstur https://github.com/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.

—
Reply to this email directly, view it on GitHub
https://github.com/nix-community/lanzaboote/pull/147#issuecomment-1585647379,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AACMZRFDAKJDA5ZYBFPARF3XKRSIPANCNFSM6AAAAAAW6MPCMY
.
You are receiving this because you authored the thread.Message ID:
@.***>

I see, then, no problem, I will find a way :-) Le sam. 10 juin 2023 à 14:31, nikstur ***@***.***> a écrit : > @nikstur <https://github.com/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. > > — > Reply to this email directly, view it on GitHub > <https://github.com/nix-community/lanzaboote/pull/147#issuecomment-1585647379>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AACMZRFDAKJDA5ZYBFPARF3XKRSIPANCNFSM6AAAAAAW6MPCMY> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

nikstur (Migrated from github.com) approved these changes

2023-06-11 15:28:07 +00:00

nikstur (Migrated from github.com) left a comment

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

SuperSandro2000 (Migrated from github.com) reviewed

2023-06-13 17:13:56 +00:00

flake.nix

SuperSandro2000 (Migrated from github.com) commented

2023-06-13 17:13:56 +00:00

              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.

```suggestion 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.