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

tracexec: Packaging improvements #310158

Merged
merged 1 commit into from
May 19, 2024
Merged

tracexec: Packaging improvements #310158

merged 1 commit into from
May 19, 2024

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented May 8, 2024

Original PR title: tracexec: init at 0.1.0

Description of changes

See https://github.com/kxxt/tracexec

❤️ co-maintainers would be very appreciated!

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.

pkgs/by-name/tr/tracexec/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/tr/tracexec/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/tr/tracexec/package.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@kxxt kxxt left a comment

Choose a reason for hiding this comment

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

Thanks for your packaging effort! I have added some review comments and the test and licenses. Apart from that, there's some extra complexity for handling different architectures: seccomp-bpf feature isn't available for riscv64 architecture due to seccompiler doesn't support it: rust-vmm/seccompiler#72 .

In general, you can take a look at my PKGBUILD at AUR: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=tracexec

And a kind reminder that in the future tests should be run single threaded, with RUST_TEST_THREADS=1

pkgs/by-name/tr/tracexec/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/tr/tracexec/package.nix Outdated Show resolved Hide resolved
@nh2
Copy link
Contributor Author

nh2 commented May 17, 2024

seccomp-bpf feature isn't available for riscv64 architecture due to seccompiler doesn't support it: rust-vmm/seccompiler#72 .

Done.

And a kind reminder that in the future tests should be run single threaded, with RUST_TEST_THREADS=1

I'm skipping over this as https://github.com/kxxt/tracexec/releases/tag/v0.2.2 mentions this as fixed.

@nh2 nh2 requested a review from DontEatOreo May 17, 2024 19:01
@nh2 nh2 mentioned this pull request May 17, 2024
13 tasks
@nh2
Copy link
Contributor Author

nh2 commented May 17, 2024

Ah, looks like tracexec was already packaged meanwhile:

I'll apply the improvements made here onto it.

@nh2 nh2 changed the title tracexec: init at 0.1.0 tracexec: Packaging improvements May 17, 2024
nh2 added a commit to nh2/nixpkgs that referenced this pull request May 17, 2024
@nh2
Copy link
Contributor Author

nh2 commented May 17, 2024

Good to merge from my side; if ofborg passes, please somebody merge.

@WilliButz
Copy link
Member

@ofborg build tracexec

@ofborg ofborg bot requested a review from fpletz May 17, 2024 20:16
@WilliButz
Copy link
Member

WilliButz commented May 17, 2024

Hmm, this seems to introduce a build failure in the check phase on aarch64-linux:

Relevant log:

buildPhase completed in 2 minutes 43 seconds
Running phase: checkPhase
Executing cargoCheckHook
++ cargo test -j 4 --profile release --target aarch64-unknown-linux-gnu --frozen -- --test-threads=4 --skip=cli::test::log_mode_without_args_works --skip=tracer::test::tracer_emits_exec_event
   Compiling proc-macro2 v1.0.81
   Compiling unicode-ident v1.0.12
   Compiling cfg-if v1.0.0
   Compiling libc v0.2.154
   Compiling autocfg v1.3.0
   Compiling memchr v2.7.2
error: linker `aarch64-linux-gnu-gcc` not found
  |
  = note: No such file or directory (os error 2)

error: could not compile `proc-macro2` (build script) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `libc` (build script) due to 1 previous error

Edit: ofborg logs: https://logs.ofborg.org/?key=nixos/nixpkgs.310158&attempt_id=2fc50c54-2fc3-4592-9531-96631b6355dd

@DontEatOreo
Copy link
Member

It would be nice to have updateScript to automate updates

passthru.updateScript = gitUpdater {
  url = "https://github.com/kxxt/tracexec";
  rev-prefix = "v";
};

Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Sorry, didn't see your PR. Thanks! ❤️

@nh2
Copy link
Contributor Author

nh2 commented May 18, 2024

Hmm, this seems to introduce a build failure in the check phase on aarch64-linux:

error: linker `aarch64-linux-gnu-gcc` not found

@WilliButz No clue about that one. Should Rust aarch64 have a working GCC out of the box?

Edit:

I succeed to build it cross, which suggest to me that maybe something not specific to this package is broken:

NIX_PATH=nixpkgs=. nix-build '<nixpkgs>' --arg crossSystem '(import <nixpkgs/lib>).systems.examples.aarch64-multiplatform' --no-out-link -A tracexec

@kxxt
Copy link
Contributor

kxxt commented May 18, 2024

Hmm, this seems to introduce a build failure in the check phase on aarch64-linux:

error: linker `aarch64-linux-gnu-gcc` not found

@WilliButz No clue about that one. Should Rust aarch64 have a working GCC out of the box?

I defined it here: https://github.com/kxxt/tracexec/blob/main/.cargo/config.toml . It's because rust defaults to linking with cc and that will fail with a recent gcc(e.g. gcc13/14) when cross-compiling from x86_64 host to aarch64 target.

@nh2
Copy link
Contributor Author

nh2 commented May 18, 2024

passthru.updateScript = gitUpdater { url = "https://github.com/kxxt/tracexec"; rev-prefix = "v"; };

@DontEatOreo I used the more generic

passthru.updateScript = nix-update-script { };

mentioned in the Tip of https://nixos.org/manual/nixpkgs/stable/#var-passthru-updateScript which works for this package and does not copy-pasting the Github URL.

Tested with:

NIX_PATH=nixpkgs=. nix-shell maintainers/scripts/update.nix --argstr package tracexec

nh2 added a commit to nh2/nixpkgs that referenced this pull request May 18, 2024
@ofborg ofborg bot requested a review from fpletz May 18, 2024 15:09
@nh2
Copy link
Contributor Author

nh2 commented May 18, 2024

I defined it here: https://github.com/kxxt/tracexec/blob/main/.cargo/config.toml . It's because rust defaults to linking with cc and that will fail with a recent gcc(e.g. gcc13/14) when cross-compiling from x86_64 host to aarch64 target.

@kxxt What would be a good solution to that?

As in, can that be written such that it does not break native (non-cross) compilation, or should we work around that in nixpkgs somehow?

(The cross-compilation of the package currently works in nixpkgs btw, I just tested it and edited put the invocation into #310158 (comment), in case that is useful for you).

@nh2
Copy link
Contributor Author

nh2 commented May 18, 2024

@kxxt

How does this work in the AUR package (which only has 0.1.0 but that already has the relevant code kxxt/tracexec@b8d29bd)? Is the compiler just always called/aliased aarch64-linux-gnu-gcc there?

@nh2
Copy link
Contributor Author

nh2 commented May 18, 2024

I suspect aarch64 worked (ofborg log) in the initial packaging because that had tests disabled (doCheck = false;) and the problem only occurs in checkPhase.

I'll disable tests for non-x86_64 archs until a solution is found.

nh2 added a commit to nh2/nixpkgs that referenced this pull request May 18, 2024
Copy link
Member

@DontEatOreo DontEatOreo left a comment

Choose a reason for hiding this comment

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

LGTM!

@kxxt
Copy link
Contributor

kxxt commented May 18, 2024

@kxxt

How does this work in the AUR package (which only has 0.1.0 but that already has the relevant code kxxt/tracexec@b8d29bd)? Is the compiler just always called/aliased aarch64-linux-gnu-gcc there?

Because when host and target architecture is the same, a compatible symlink is there, at least on Arch Linux(and Ubuntu or GitHub runner image) . (e.g. aarch64-linux-gnu-gcc symlinked to gcc).

Comment on lines +53 to +54
install -Dm644 LICENSE -t "$out/share/licenses/${pname}/"
install -Dm644 THIRD_PARTY_LICENSES.HTML -t "$out/share/licenses/${pname}/"
Copy link
Member

Choose a reason for hiding this comment

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

pname probably shouldn't be reused for the license directory

@nh2
Copy link
Contributor Author

nh2 commented May 19, 2024

With disabled tests on aarch64, ofborg passes, merging.

@nh2 nh2 merged commit 8cd1bf6 into NixOS:master May 19, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants