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.

Sign up for GitHub

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

Jump to bottom

tests.buildRustCrate: Fix after hashing method change #105379

Merged
merged 2 commits into from
Dec 19, 2020
Merged

tests.buildRustCrate: Fix after hashing method change #105379

merged 2 commits into from
Dec 19, 2020

Conversation

Ericson2314
Copy link
Member

Motivation for this change

As @lopsided98 points out in #105305, we're probably better off not asserting exact hashes, but that is a more involved fix.

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.

@Ericson2314 Ericson2314 mentioned this pull request Nov 29, 2020
Merged
10 tasks
@ofborg ofborg bot added 6.topic: rust 8.has: clean-up 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 Nov 29, 2020
@lopsided98
Copy link
Contributor

This only fixes the tests for x86_64-linux, every other platform is still broken.

@Ericson2314
Copy link
Member Author

Oh good point, didn't think of that.

andir
andir requested changes Nov 30, 2020
View reviewed changes
Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

nixpkgs/pkgs/build-support/rust/build-rust-crate/test/default.nix

Lines 161 to 164 in 8275fc2

diff -u ${expectedFilesFile} $actualFiles |\
tail -n +3 |\
sed -e 's/^/ /'
exit 1

We can probably just exchange this for a loop in which we run state against the given expected file name (which can contain wildcards)?

@Ericson2314
Copy link
Member Author

@andir that sounds good.

@Ericson2314
buildRustCrateTests: Move to tests.buildRustCrate
5d2a20c 
I think it is preferable to separate the tests from the "real" packages.
@Ericson2314
tests.buildRustCrate: Fix after hashing method change
ddeef0d 
As @lopsided98 points out in #105305, since the hashes are now target
sensative, and until we find reason to actually care to test what they
are exactly, we are best just normalizing them away in the tests.
@Ericson2314 Ericson2314 force-pushed the fix-buildRustCrateTests branch from 8275fc2 to ddeef0d Compare December 19, 2020 19:06
@Ericson2314 Ericson2314 changed the title tests.buildRustCrate: Fix rlibc hashes tests.buildRustCrate: Fix after hashing method change Dec 19, 2020
@Ericson2314
Copy link
Member Author

We can probably just exchange this for a loop in which we run state against the given expected file name (which can contain wildcards)?

I liked the idea of running diff because it's easiest to diagnose in the failure case. So i didn't really know what to do. But then I realized it was easy: just normalize before diffing.

This should now be ready to go.

@Ericson2314 Ericson2314 merged commit 86c1825 into master Dec 19, 2020
@Ericson2314 Ericson2314 deleted the fix-buildRustCrateTests branch December 19, 2020 19:49
@andir
Copy link
Member

andir commented Dec 20, 2020

Mhm, hydra reports the job as begin removed now: https://hydra.nixos.org/eval/1636253#tabs-removed

@Ericson2314
Copy link
Member Author

@andir see first commit it's got a new attr path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andir andir andir requested changes

@lopsided98 lopsided98 Awaiting requested review from lopsided98

@zowoq zowoq Awaiting requested review from zowoq

Assignees
No one assigned
Labels
6.topic: rust 8.has: clean-up 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ericson2314 @lopsided98 @andir