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

bandwhich: patch dependency to avoid panics #135804

Merged
merged 1 commit into from
Oct 3, 2021

Conversation

alarsyo
Copy link
Contributor

@alarsyo alarsyo commented Aug 26, 2021

Panic messages rendered the output unreadable, these panics came from a bug in the linked-hash-map dependency. The patch file is huge because Cargo.lock format changed, but the only thing I ran was:

cargo update -p linked-hash-map

See imsnif/bandwhich#192 (comment) for the comment where the fix is mentioned

Motivation for this change
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 via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.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.

@SuperSandro2000
Copy link
Member

Can you create a PR with that and fetch the PR from there? I don't think we want to include 3k lines just for a patch.

Panic messages rendered the output unreadable, these panics came from a
bug in the linked-hash-map dependency. The patch file is huge because
Cargo.lock format changed, but the only thing I ran was:

    cargo update -p linked-hash-map
@alarsyo alarsyo force-pushed the bandwhich-dep-bump branch from 2f4d1a0 to 8798aaa Compare August 26, 2021 15:18
@ofborg ofborg bot requested a review from Ma27 August 26, 2021 15:31
@Ma27
Copy link
Member

Ma27 commented Aug 26, 2021

Hmm, why did you close the upstream PR?

@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 26, 2021

@Ma27 the goal of opening the PR was to host the patch; the version in nixpkgs is v0.20.0 but their master branch has evolved since 0.20.0, and the patch needed for nixpkgs doesn't apply on their master branch, so I'd need to open another PR anyway (or rebase the existing PR branch now that the original patch is hosted somewhere)

@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 26, 2021

there's also imsnif/bandwhich#192 which does include a dependency update, so that's covered there if it ever gets merged

Copy link
Contributor

@sbruder sbruder left a comment

Choose a reason for hiding this comment

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

Works and fixes the panics.

Also, when using an older cargo version to update the dependency (e. g. by running nix run nixpkgs/nixos-20.03#cargo -- update -p linked-hash-map) the patch is a lot shorter (49 lines):

Patch
diff --git a/Cargo.lock b/Cargo.lock
index 8244d96..e960e1e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -777,7 +777,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"

 [[package]]
 name = "linked-hash-map"
-version = "0.5.2"
+version = "0.5.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"

 [[package]]
@@ -809,7 +809,7 @@ name = "lru-cache"
 version = "0.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
- "linked-hash-map 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)",
+ "linked-hash-map 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)",
 ]

 [[package]]
@@ -1588,7 +1588,7 @@ version = "0.8.11"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "dtoa 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)",
- "linked-hash-map 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)",
+ "linked-hash-map 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "serde 1.0.102 (registry+https://github.com/rust-lang/crates.io-index)",
  "yaml-rust 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
@@ -2105,7 +2105,7 @@ name = "yaml-rust"
 version = "0.4.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
- "linked-hash-map 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)",
+ "linked-hash-map 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)",
 ]

 [[package]]
@@ -2210,7 +2210,7 @@ dependencies = [
 "checksum libc 0.2.76 (registry+https://github.com/rust-lang/crates.io-index)" = "755456fae044e6fa1ebbbd1b3e902ae19e73097ed4ed87bb79934a867c007bc3"
 "checksum libflate 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "e9bac9023e1db29c084f9f8cd9d3852e5e8fddf98fb47c4964a0ea4663d95949"
 "checksum libflate_lz77 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3286f09f7d4926fc486334f28d8d2e6ebe4f7f9994494b6dab27ddfad2c9b11b"
-"checksum linked-hash-map 0.5.2 (registry+https://github.com/rust-lang/crates.io-index)" = "ae91b68aebc4ddb91978b11a1b02ddd8602a05ec19002801c5666000e05e0f83"
+"checksum linked-hash-map 0.5.4 (registry+https://github.com/rust-lang/crates.io-index)" = "7fb9b38af92608140b86b693604b9ffcc5824240a484d1ecd4795bacb2fe88f3"
 "checksum lock_api 0.3.4 (registry+https://github.com/rust-lang/crates.io-index)" = "c4da24a77a3d8a6d4862d95f72e6fdb9c09a643ecdb402d754004a557f2bec75"
 "checksum log 0.3.9 (registry+https://github.com/rust-lang/crates.io-index)" = "e19e8d5c34a3e0e2223db8e060f9e8264aeeb5c5fc64a4ee9965c062211c024b"
 "checksum log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)" = "14b6052be84e6b71ab17edffc2eeabf5c2c3ae1fdb464aae35ac50c67a44e1f7"

Copy link
Member

@Br1ght0ne Br1ght0ne left a comment

Choose a reason for hiding this comment

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

I like this PR as is and I'm all for merging. You can also use @sbruder's patch at your discretion. I'll merge this PR on Tuesday if nothing comes up.

Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

Works and fixes the panics for me.

@SuperSandro2000 SuperSandro2000 merged commit d7fde91 into NixOS:master Oct 3, 2021
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.

6 participants