Skip to content

cargo-llvm-cov: init at 0.4.13#185183

Closed
matthiasbeyer wants to merge 1 commit intoNixOS:masterfrom
matthiasbeyer:add-cargo-llvm-cov
Closed

cargo-llvm-cov: init at 0.4.13#185183
matthiasbeyer wants to merge 1 commit intoNixOS:masterfrom
matthiasbeyer:add-cargo-llvm-cov

Conversation

@matthiasbeyer
Copy link
Contributor

Closes #170938
Supersedes #170938

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@matthiasbeyer matthiasbeyer changed the title cargo-llvm-cov: init at 0.3.0 cargo-llvm-cov: init at 0.4.13 Aug 5, 2022
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Aug 5, 2022
@ofborg ofborg bot requested a review from DieracDelta August 5, 2022 06:13
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Aug 5, 2022
Copy link
Member

@DieracDelta DieracDelta left a comment

Choose a reason for hiding this comment

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

LGTM, the fetchcrate seems like it fixes ofborg. Excited to see this in nixpkgs!

@matthiasbeyer
Copy link
Contributor Author

@FliegendeWurst unfortunately, your workaround does not work anymore with 0.4.14... care to investigate?

Copy link
Member

@winterqt winterqt left a comment

Choose a reason for hiding this comment

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

What I'm confused about is that this should have never worked in the first place, for a few reasons:

  1. Your hash for the crate source is wrong (off by one character).
  2. Your patch for copying the Cargo.lock doesn't apply to the dependency tarball generation, which means the build still fails.
  3. The tests depend on a nightly compiler, which we don't ship (and rustPlatform doesn't use).

Due to these reasons, I propose the following solution, which I have locally and can push if you'd like (or you can make the changes yourself):

  1. Only use the crate source, we don't care about the tests fixtures
  2. rm test/tests.rs in postPatch, to stop the tests that require nightly from running

Making these changes locally has this building successfully for me.

@matthiasbeyer
Copy link
Contributor Author

With your changes, this fails now with:

rm: cannot remove 'test/tests.rs': No such file or directory

@winterqt
Copy link
Member

winterqt commented Aug 8, 2022

Ugh, sorry, made a typo in my original message and didn't notice before repeating it: it should be rm tests/test.rs.

@matthiasbeyer
Copy link
Contributor Author

I'm gonna use fixup patches for now and squash once all of you approve.

Co-authored-by: Justin Restivo <justin@restivo.me>
Co-authored-by: FliegendeWurst <2012gdwu+github@posteo.de>
Co-authored-by: Winter <winter@winter.cafe>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@bobby285271 bobby285271 added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Aug 23, 2022
@DieracDelta DieracDelta mentioned this pull request Sep 5, 2022
13 tasks
@DieracDelta
Copy link
Member

Any chance we get this merged? I would love to drop my overlays for this and pull directly from nixpkgs.

@bobby285271 bobby285271 added 12.approvals: 1 This PR was reviewed and approved by one person. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Sep 18, 2022
@winterqt winterqt mentioned this pull request Oct 24, 2022
13 tasks
'';

nativeBuildInputs = [ makeBinaryWrapper ];
buildInputs = [ libllvm ];
Copy link
Member

Choose a reason for hiding this comment

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

Should this use the same libllvm as rustc (e.g. rustPlatform.rust.rustc.llvmPackages.libllvm)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is different because it's an older version, but I didn't need to add any buildInputs at all to get 0.5.0 to build/function.

Copy link
Member

@winterqt winterqt Oct 24, 2022

Choose a reason for hiding this comment

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

Yeah, looks like libllvm was never required, at least on 0.4.13.

@DieracDelta Do you remember why you added it, out of curiosity?

Copy link
Member

Choose a reason for hiding this comment

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

I have no recollection of adding this. If it works without libllvm, then let's drop the dependency 👍

@winterqt
Copy link
Member

winterqt commented Oct 24, 2022

I'm happy to merge this after #185183 (comment) is figured out. I don't see why we shouldn't use the same version (smaller closure size and all), but maybe it matters?

Apologies for forgetting about this PR.

@CobaltCause
Copy link
Contributor

Is there any reason not to update this PR to init at upstream's 0.5.0?

@winterqt
Copy link
Member

winterqt commented Oct 24, 2022

I'm going to close this in favor of #197478, because it has all the tests working, and does not wrap the binary (and is the latest version).

Thanks to everyone who got this implementation where it was, and if you can see anything in there that's worth a review, don't hesitate to comment there :)

@winterqt winterqt closed this Oct 24, 2022
@matthiasbeyer matthiasbeyer deleted the add-cargo-llvm-cov branch October 24, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants