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

cargo-llvm-cov: 0.5.33 -> 0.5.35 #261650

Closed

Conversation

matthiasbeyer
Copy link
Contributor

Description of changes

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.11 Release Notes (or backporting 23.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
  • Fits CONTRIBUTING.md.

@gepbird
Copy link
Contributor

gepbird commented Oct 17, 2023

Result of nixpkgs-review pr 261650 run on x86_64-linux 1

1 package failed to build:
  • cargo-llvm-cov
1 test failed:
failures:

---- bin_crate stdout ----
thread 'bin_crate' panicked at 'called Result::unwrap() on an Err value: bin_crate/bin_crate

Caused by:
0: could not execute process cd "/build/source/tests/fixtures/crates/bin_crate" && "git" "ls-files"
1: No such file or directory (os error 2)', tests/test.rs:38:14

@matthiasbeyer
Copy link
Contributor Author

Yep, help would be appreciated!

@CobaltCause
Copy link
Contributor

@matthiasbeyer matthiasbeyer changed the title cargo-llvm-cov: 0.5.33 -> 0.5.34 cargo-llvm-cov: 0.5.33 -> 0.5.35 Oct 24, 2023
@matthiasbeyer
Copy link
Contributor Author

Still not working for the same reasons as before.

@CobaltCause
Copy link
Contributor

CobaltCause commented Oct 30, 2023

This part of the release broke the tests for us: taiki-e/cargo-llvm-cov@v0.5.33...v0.5.34#diff-1e945b89ea291b412e6b40b574d24c7335a61b6b3745d734e946975134c44cadR131

This helps but doesn't fix it:

diff --git a/pkgs/development/tools/rust/cargo-llvm-cov/default.nix b/pkgs/development/tools/rust/cargo-llvm-cov/default.nix
index b384187c8d14..33251230b199 100644
--- a/pkgs/development/tools/rust/cargo-llvm-cov/default.nix
+++ b/pkgs/development/tools/rust/cargo-llvm-cov/default.nix
@@ -21,6 +21,7 @@
 , fetchFromGitHub
 , rustPlatform
 , rustc
+, git
 }:
 
 let
@@ -54,7 +55,8 @@ rustPlatform.buildRustPackage {
     inherit owner;
     repo = pname;
     rev = "v${version}";
-    sha256 = "sha256-ux4vVpejMkHwXxYS+LGoJufb4CiBHU7jjLX8glubnVU=";
+    sha256 = "sha256-059k2z+ZtpWgZjoFZKIHF+OHYnBcUUO/4REWW5XRHkw=";
+    leaveDotGit = true;
   };
 
   # Upstream doesn't include the lockfile so we need to add it back
@@ -69,6 +71,10 @@ rustPlatform.buildRustPackage {
   LLVM_COV = "${llvm}/bin/llvm-cov";
   LLVM_PROFDATA = "${llvm}/bin/llvm-profdata";
 
+  nativeCheckInputs = [
+    git
+  ];
+
   meta = {
     inherit homepage;
     changelog = homepage + "/blob/v${version}/CHANGELOG.md";

After applying that patch, I'm now I'm seeing errors like:

---- virtual1 stdout ----
thread 'virtual1' panicked at 'assertion failed: `self.status.success()`:

STDOUT:
------------------------------------------------------------

------------------------------------------------------------

STDERR:
------------------------------------------------------------
error: process didn't exit successfully: `/nix/store/9vxskkmmmqh2r72x63g2rr7vf93da577-cargo-1.72.0/bin/.cargo-wrapped locate-project --message-format plain` (exit status: 101)
--- stderr
error: could not find `Cargo.toml` in `/build/.tmp47f2rX` or any parent directory


------------------------------------------------------------
', tests/test.rs:37:9

Doing some nix develop .#cargo-llvm-cov and cargoCheckHook and println!() debugging suggests these errors are because nothing is getting copied into the temporary directories. Running git ls-files by hand inside source writes nothing to stdout. Don't know why (yet?).

@CobaltCause
Copy link
Contributor

CobaltCause commented Oct 30, 2023

The secret sauce was

preCheck = ''
  git restore --staged .
'';

because for some reason doing fetchFromGitHub { leaveDotGit = true; ... } leaves every file staged as deleted but then also still in the working tree as untracked. You can see this for yourself by doing e.g.

preCheck = ''
  git status
  exit 1
'';

and then looking at the logs. Pretty surprising behavior, if you ask me. Anyway, this matters for cargo-llvm-cov because its test suite relies on git ls-files to figure out which files to copy to generate the test projects.

Click to expand fully working patch
diff --git a/pkgs/development/tools/rust/cargo-llvm-cov/default.nix b/pkgs/development/tools/rust/cargo-llvm-cov/default.nix
index b384187c8d14..d9a1d5c10d07 100644
--- a/pkgs/development/tools/rust/cargo-llvm-cov/default.nix
+++ b/pkgs/development/tools/rust/cargo-llvm-cov/default.nix
@@ -21,6 +21,7 @@
 , fetchFromGitHub
 , rustPlatform
 , rustc
+, git
 }:
 
 let
@@ -54,7 +55,8 @@ rustPlatform.buildRustPackage {
     inherit owner;
     repo = pname;
     rev = "v${version}";
-    sha256 = "sha256-ux4vVpejMkHwXxYS+LGoJufb4CiBHU7jjLX8glubnVU=";
+    sha256 = "sha256-059k2z+ZtpWgZjoFZKIHF+OHYnBcUUO/4REWW5XRHkw=";
+    leaveDotGit = true;
   };
 
   # Upstream doesn't include the lockfile so we need to add it back
@@ -69,6 +71,14 @@ rustPlatform.buildRustPackage {
   LLVM_COV = "${llvm}/bin/llvm-cov";
   LLVM_PROFDATA = "${llvm}/bin/llvm-profdata";
 
+  nativeCheckInputs = [
+    git
+  ];
+
+  preCheck = ''
+    git restore --staged .
+  '';
+
   meta = {
     inherit homepage;
     changelog = homepage + "/blob/v${version}/CHANGELOG.md";

Also, https://github.com/taiki-e/cargo-llvm-cov/releases/tag/v0.5.36 exists now.

@matthiasbeyer
Copy link
Contributor Author

Can you send the patches to me or give me an URL to pull from?

I also am in the process of packaging the update to 0.5.36.

Signed-off-by: Matthias Beyer <[email protected]>
@CobaltCause
Copy link
Contributor

CobaltCause commented Oct 31, 2023

Can you send the patches to me or give me an URL to pull from?

I put it in #261650 (comment), you should be able to just copy it to your clipboard and then do wl-paste -n | patch -p1 assuming Wayland. The hash change will probably get rejected though so you'll have to fix that. Or you can just apply it manually (like, by editing the file and using the patch as a reference).


Also, you should include this patch in the first commit and then carry it through all the other commits, so that each one can build properly. Keep in mind you'll have to update the hash every time.

@matthiasbeyer
Copy link
Contributor Author

Yeah please provide something I can pull from. That's less hassle for everyone and even preserves credit to you.

If I apply the changes from your above fix, the issue is not fixed (fatal: not a git repository (or any parent up to mount point /)).

@CobaltCause
Copy link
Contributor

I think that means you forgot to change the hash for fetchFromGitHub and you're getting a stale version of the FOD from before leaveDotGit = true; was set.

Anyway, I opened #264606 since it takes 2 extra seconds to open a PR after doing the work of carrying the patch through all the updates, making commits for this, and pushing them somewhere.

@CobaltCause
Copy link
Contributor

Hey, matthiasbeyer, sorry for the hostility in the previous comment. Also, apologies for putting any nixpkgs committers who saw this into the awkward position of trying to figure out whether to go ahead with one of these, and if so, which one.

To get things moving again, do you want to pull the changes/commits from #264606 into this PR and have me close that, or do you want to close this one in favor of that one? Personally, I'm okay with either option, I have no preference.

@matthiasbeyer
Copy link
Contributor Author

Don't worry too much about it.
I test built, approved and merged the other PR and am closing this.

@matthiasbeyer matthiasbeyer deleted the update-cargo-llvm-cov branch November 15, 2023 06:45
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

3 participants