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-cache: init at 0.3.4 #77310

Merged
merged 1 commit into from Aug 23, 2020
Merged

cargo-cache: init at 0.3.4 #77310

merged 1 commit into from Aug 23, 2020

Conversation

Br1ght0ne
Copy link
Member

@Br1ght0ne Br1ght0ne commented Jan 8, 2020

Motivation for this change

https://users.rust-lang.org/t/cargo-cache-0-3-4-faster-cargo-home-caching-on-ci
The tag for 0.3.4 is missing.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This change is Reviewable

Co-authored-by: Evan Stoll <evanjsx@gmail.com>
@Br1ght0ne
Copy link
Member Author

@GrahamcOfBorg build cargo-cache

Copy link
Member

@lukateras lukateras left a comment

Choose a reason for hiding this comment

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

Unfortunately, tests are failing:

running 1 test
test run_tests ... FAILED

failures:

---- run_tests stdout ----
REGEX:
CARGO_HOME .*./xyxyxxxyyyxxyxyxqwertywasd. is not an existing directory!\n
OUTPUT:
CARGO_HOME "/build/source/./xyxyxxxyyyxxyxyxqwertywasd" is not an existing directory!

src = fetchFromGitHub {
owner = "matthiaskrgr";
repo = pname;
# TODO: use v${version} when a tag is available
Copy link
Member

Choose a reason for hiding this comment

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

I requested this at matthiaskrgr/cargo-cache#79 and maintainer immediately tagged the release. It should now be available as 0.3.4, without the v.

Comment on lines 21 to 23
--skip CargoCachePaths \
--skip alternative_registry_works \
--skip cargo_new_and_run_local
Copy link
Member

Choose a reason for hiding this comment

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

After cursory look at these tests, my impression is that alternative_registry_works requires internet access, and the other two could probably be made to work by setting HOME = "."?

Choose a reason for hiding this comment

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

Hi, yes, a couple of the cargo-cache tests require internet because I download crates/registry indices or initialize a new $CARGO_HOME at test-runtime.

@matthiaskrgr
Copy link

So, I guess you are running cargo test in some kind of sandboxed environment which has no connection to the outside world?

I did a quick experiment and these should be the tests that require internet connectivity:

CARGO_HOME_subdirs_are_known
alternative_registry_works
remove_dirs
build_and_check_size_test
spurious_files_in_cache_test

Maybe I can add some kind of feature flag that only runs offline-tests and skips those.

@lukateras
Copy link
Member

Yes, we are building in a sandboxed environment. Having a feature flag for that would be great! But just knowing the affected tests is very useful, thank you :)

@matthiaskrgr
Copy link

So, I added the offline_tests feature now, so in git, or after next release, you should be able to run cargo test --features offline_tests in your sandboxed env!

@Br1ght0ne
Copy link
Member Author

@matthiaskrgr I will update this PR after the release. Thank you!

@evanjs
Copy link
Member

evanjs commented Jun 28, 2020

I did not see this before creating matthiaskrgr/cargo-cache#84
The good (for me :D) news is that the tests I mentioned in said issue do still fail.

The diff I tested
diff --git a/pkgs/development/tools/rust/cargo-cache/default.nix b/pkgs/development/tools/rust/cargo-cache/default.nix
index 8dee10fe2f3..5215f971cbe 100644
--- a/pkgs/development/tools/rust/cargo-cache/default.nix
+++ b/pkgs/development/tools/rust/cargo-cache/default.nix
@@ -7,26 +7,22 @@ rustPlatform.buildRustPackage rec {
   src = fetchFromGitHub {
     owner = "matthiaskrgr";
     repo = pname;
-    # TODO: use v${version} when a tag is available
-    rev = "f3d113cc74644c4581c127e16b03e319677d669f";
-    sha256 = "1bc9c1r964w0766k3yhf79d30ldv37vyiji8wnqspk5sl2gzn0iq";
+    rev = "v${version}";
+    sha256 = "0xhm7jlqq9nl1r8plx51x7aisza665f28d9h649b62904mx2ad7k";
   };
 
-  cargoSha256 = "1wzl8xvn3i5qgc4zmdqdh4kd8ww2ll2g14w4rf31i9lvq1ipa031";
+  cargoSha256 = "09vsalxh07bk1d48iyl0ncyzsscxlnf0dn96a2vbb1lin2qa2axm";
 
   buildInputs = lib.optional stdenv.isDarwin libiconv;
 
   checkPhase = ''
-    cargo test -- \
-      --skip CargoCachePaths \
-      --skip alternative_registry_works \
-      --skip cargo_new_and_run_local
+    cargo test --features offline_tests
   '';
 
   meta = with lib; {
-    description = "Manage cargo cache";
+    description = "manage cargo cache (\${CARGO_HOME}, ~/.cargo/), print sizes of dirs and remove dirs selectively";
     homepage = "https://github.com/matthiaskrgr/cargo-cache";
-    license = with licenses; [ asl20 mit ];
+    license = with licenses; [ asl20 /* or */ mit ];
     maintainers = with maintainers; [ filalex77 ];
   };
 }
The still-failing tests
cargo-cache> failures:
cargo-cache> ---- library::libtests::test_CargoCachePaths_paths stdout ----
cargo-cache> thread 'library::libtests::test_CargoCachePaths_paths' panicked at 'assertion failed: `(left == right)`
cargo-cache> left: `["source", "target", "cargo_home_cargo_cache_paths"]`,
cargo-cache> right: `["cargo-cache", "target", "cargo_home_cargo_cache_paths"]`', src/test_helpers.rs:51:5
cargo-cache> note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
cargo-cache> ---- library::libtests::test_CargoCachePaths_print stdout ----
cargo-cache> thread 'library::libtests::test_CargoCachePaths_print' panicked at 'assertion failed: `(left == right)`
cargo-cache> left: `["source", "target", "cargo_home_cargo_cache_paths_print"]`,
cargo-cache> right: `["cargo-cache", "target", "cargo_home_cargo_cache_paths_print"]`', src/test_helpers.rs:51:5
cargo-cache> failures:
cargo-cache> library::libtests::test_CargoCachePaths_paths
cargo-cache> library::libtests::test_CargoCachePaths_print
cargo-cache> test result: FAILED. 80 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out
cargo-cache> error: test failed, to rerun pass '--bin cargo-cache'
builder for '/nix/store/c5ifzay45wcr0pqzx2pqmjksq3p0l68y-cargo-cache-0.4.3.drv' failed with exit code 101; last 10 log lines:
   right: `["cargo-cache", "target", "cargo_home_cargo_cache_paths_print"]`', src/test_helpers.rs:51:5
  
  
  failures:
      library::libtests::test_CargoCachePaths_paths
      library::libtests::test_CargoCachePaths_print
  
  test result: FAILED. 80 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out

@evanjs
Copy link
Member

evanjs commented Jul 28, 2020

Huge derp. checkFlagsArray works for cargo, too....
In short, with checkFlagsArray = [ "offline_tests"];, everything seems to work fine 😅

Nix Expression for 0.4.3
{ stdenv, lib, fetchFromGitHub, rustPlatform, libiconv }:

rustPlatform.buildRustPackage rec {
  pname = "cargo-cache";
  version = "0.4.3";

  src = fetchFromGitHub {
    owner = "matthiaskrgr";
    repo = pname;
    rev = version;
    sha256 = "0xhm7jlqq9nl1r8plx51x7aisza665f28d9h649b62904mx2ad7k";
  };

  cargoSha256 = "0gqhyav3dk8rbkcnq9m66z8gv60ipvc556zri6m1hps5jrffsfik";

  buildInputs = lib.optional stdenv.isDarwin libiconv;

  checkFlagsArray = [ "offline_tests"];

  meta = with lib; {
    description = "manage cargo cache (\${CARGO_HOME}, ~/.cargo/), print sizes of dirs and remove dirs selectively";
    homepage = "https://github.com/matthiaskrgr/cargo-cache";
    license = with licenses; [ asl20 /* or */ mit ];
    maintainers = with maintainers; [ filalex77 evanjs ];
  };
}
Diff of my expression for 0.4.3 against this PR
diff --git a/pkgs/development/tools/rust/cargo-cache/default.nix b/pkgs/development/tools/rust/cargo-cache/default.nix
index 8dee10fe2f3..537419a6d6b 100644
--- a/pkgs/development/tools/rust/cargo-cache/default.nix
+++ b/pkgs/development/tools/rust/cargo-cache/default.nix
@@ -2,31 +2,25 @@

 rustPlatform.buildRustPackage rec {
   pname = "cargo-cache";
-  version = "0.3.4";
+  version = "0.4.3";

   src = fetchFromGitHub {
     owner = "matthiaskrgr";
     repo = pname;
-    # TODO: use v${version} when a tag is available
-    rev = "f3d113cc74644c4581c127e16b03e319677d669f";
-    sha256 = "1bc9c1r964w0766k3yhf79d30ldv37vyiji8wnqspk5sl2gzn0iq";
+    rev = version;
+    sha256 = "0xhm7jlqq9nl1r8plx51x7aisza665f28d9h649b62904mx2ad7k";
   };

-  cargoSha256 = "1wzl8xvn3i5qgc4zmdqdh4kd8ww2ll2g14w4rf31i9lvq1ipa031";
+  cargoSha256 = "0gqhyav3dk8rbkcnq9m66z8gv60ipvc556zri6m1hps5jrffsfik";

   buildInputs = lib.optional stdenv.isDarwin libiconv;

-  checkPhase = ''
-    cargo test -- \
-      --skip CargoCachePaths \
-      --skip alternative_registry_works \
-      --skip cargo_new_and_run_local
-  '';
-
+  checkFlagsArray = [ "offline_tests"];
+
   meta = with lib; {
-    description = "Manage cargo cache";
+    description = "manage cargo cache (\${CARGO_HOME}, ~/.cargo/), print sizes of dirs and remove dirs selectively";
     homepage = "https://github.com/matthiaskrgr/cargo-cache";
-    license = with licenses; [ asl20 mit ];
+    license = with licenses; [ asl20 /* or */ mit ];
     maintainers = with maintainers; [ filalex77 ];
   };
 }

I think this should be good to merge given the above diff.

@Br1ght0ne
Copy link
Member Author

Thank you @evanjs for your help! I applied your diff in 382d612 and added you as co-author. Thanks again!

@evanjs
Copy link
Member

evanjs commented Aug 18, 2020

0.5.1 seems to work fine without any further changes.

diff --git a/pkgs/development/tools/rust/cargo-cache/default.nix b/pkgs/development/tools/rust/cargo-cache/default.nix
index 8c9bc4bffe5..f3756fe9468 100644
--- a/pkgs/development/tools/rust/cargo-cache/default.nix
+++ b/pkgs/development/tools/rust/cargo-cache/default.nix
@@ -2,16 +2,16 @@
 
 rustPlatform.buildRustPackage rec {
   pname = "cargo-cache";
-  version = "0.4.3";
+  version = "0.5.1";
 
   src = fetchFromGitHub {
     owner = "matthiaskrgr";
     repo = pname;
     rev = version;
-    sha256 = "0xhm7jlqq9nl1r8plx51x7aisza665f28d9h649b62904mx2ad7k";
+    sha256 = "02d593w1x8160p4m3jwm1dyvv383cy7njijlcaw49jczxv5isqbi";
   };
 
-  cargoSha256 = "0gqhyav3dk8rbkcnq9m66z8gv60ipvc556zri6m1hps5jrffsfik";
+  cargoSha256 = "0wpg0phfavd8fxl36nvanldiysy5xpk99qpnsc1jd6dw4ml01mbi";
 
   buildInputs = lib.optional stdenv.isDarwin libiconv;
Cargo cache '/home/evanjs/.cargo':

Total:                           724.46 MB
  22 installed binaries:          95.66 MB
  Registry:                      591.76 MB
    2 registry indices:          275.47 MB
    2058 crate archives:         194.77 MB
    45 crate source checkouts:   121.52 MB
  Git db:                         37.03 MB
    13 bare git repos:            36.27 MB
    3 git repo checkouts:        765.72 KB

Clearing cache...

Size changed from 724.46 MB to 602.18 MB (-122.28 MB, -16.87%)

@jonringer @marsam think we could get this merged after it's bumped to 0.5.1?

@marsam
Copy link
Contributor

marsam commented Aug 23, 2020

@GrahamcOfBorg eval

@marsam marsam merged commit 8e3035e into NixOS:master Aug 23, 2020
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

5 participants