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

crosvm: warn that NIX_CFLAGS_COMPILE will break the build #204694

Closed
wants to merge 1 commit into from
Closed

crosvm: warn that NIX_CFLAGS_COMPILE will break the build #204694

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 5, 2022

CC @alyssais

Description of changes

(note: this seems to affect only arm, not x86)

This was a bit of a headache to track down, so I'd like to add a warnIf to save others the hassle.

As of crosvm-107, if you put anything in NIX_CFLAGS_COMPILE (including ARM model-specific spectre mitigation flags), build.rs will produce an empty constants.json, which will then cause the bpf files to not be produced, which will result in errors like this at the very end of the build process:

error: couldn't read /build/crosvm-5a49a83/target/aarch64-unknown-linux-gnu/release/build/crosvm-44bfa298077e14a2/out/policy_output/9p_device.bpf: No su
 --> /build/crosvm-5a49a83/target/aarch64-unknown-linux-gnu/release/build/crosvm-44bfa298077e14a2/out/bpf_includes.in:2:15
  |
2 | ...", include_bytes!("/build/crosvm-5a49a83/target/aarch64-unknown-linux-gnu/release/build/crosvm-44bfa298077e14a2/out/policy_output/9p_device.bpf")
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `include_bytes` (in Nightly builds, run with -Z macro-backtrace for more info)
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.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 5, 2022
@ghost
Copy link
Author

ghost commented Dec 5, 2022

Also, it is sad that buildRustPackage does not allow finalAttrs. ☹️ ☹️ ☹️

@lukegb lukegb changed the title crosvm: warn that NIX_CLFAGS_COMPILE will break the build crosvm: warn that NIX_CFLAGS_COMPILE will break the build Dec 5, 2022
This was a bit of a headache to track down, so I'd like to add a
`warnIf` to save others the hassle.

If you put anything in `NIX_CFLAGS_COMPILE` (including ARM
model-specific spectre mitigation flags), `build.rs` will produce an
empty `constants.json`, which will then cause the `bpf` files to not
be produced, which will result in errors like this at the very end
of the build process:

```
error: couldn't read /build/crosvm-5a49a83/target/aarch64-unknown-linux-gnu/release/build/crosvm-44bfa298077e14a2/out/policy_output/9p_device.bpf: No su
 --> /build/crosvm-5a49a83/target/aarch64-unknown-linux-gnu/release/build/crosvm-44bfa298077e14a2/out/bpf_includes.in:2:15
  |
2 | ...", include_bytes!("/build/crosvm-5a49a83/target/aarch64-unknown-linux-gnu/release/build/crosvm-44bfa298077e14a2/out/policy_output/9p_device.bpf")
  |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `include_bytes` (in Nightly builds, run with -Z macro-backtrace for more info)
```
@ghost
Copy link
Author

ghost commented Dec 6, 2022

@lukegb thank you. I think it's time to retire my glitchy dying keyboard before it embarrasses me again...

@ghost
Copy link
Author

ghost commented Dec 7, 2022

I found the root cause: upstream stopped shipping the .policy files with the source.

Generating the .policy files at build time meand that this package uses both gcc (mostly) and clang (in build.rs, to generate the constants.json file by running some python scripts on the LLVM IR). This is the only package I know of that uses both.

GCC and Clang disagree on what names to use for a lot of the ARM-specific flags, so I maintain two sets of flags and pick one or the other based on which C compiler a package uses. Unfortunately Nixpkgs doesn't seem to provide any way to indicate that certain flags in NIX_CFLAGS_COMPILE are gcc-only or clang-only.

Ugh.

PS it is unreasonably difficult to get cargo to not discard the stderr from subprocesses of build.rs. ☹️

@ghost ghost closed this Dec 7, 2022
@ghost
Copy link
Author

ghost commented Dec 8, 2022

@ghost ghost deleted the crosvm-warn-cflags branch December 9, 2022 05:06
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0 participants