Skip to content

lib.fetchers: add hash-normalization helpers#342072

Merged
nicoonoclaste merged 15 commits intoNixOS:masterfrom
nicoonoclaste:sha-to-SRI/fetchers
Sep 17, 2024
Merged

lib.fetchers: add hash-normalization helpers#342072
nicoonoclaste merged 15 commits intoNixOS:masterfrom
nicoonoclaste:sha-to-SRI/fetchers

Conversation

@nicoonoclaste
Copy link
Copy Markdown
Contributor

@nicoonoclaste nicoonoclaste commented Sep 15, 2024 •
edited
Loading

Description of changes

  • Add functions to lib.fetchers, intended to deduplicate the logic needed for Tracking: deprecate sha256 attribute in fetchers in favor of hash = "<SRI hash>" #325892
    • normalizeHash converts an attrset containing one of hash, sha256, ... into one containing outputHash{,Algo} ;
    • withNormalizedHash wraps a function with normalizeHash, updating its signature :
      it automatically converts a fetcher which accepts outputHash{, Algo} into the current convention.
    • Comply with current documentation conventions.
    • Optimize until there is no perf. regression, and even a 6% improvement as of 582439bfc2793729823e7d993149df170be01dff.
  • Wrap fetchRepoProject in withNormalizedHash, and update amdvlk to use the hash attribute as working example.
  • Modify buildBazelPackage to accept fetchAttrs.hash (using normalizedHash) and update a few packages as working examples.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested compilation of all affected packages using nixpkgs-review.
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: lib The Nixpkgs function library labels Sep 15, 2024
nicoonoclaste
nicoonoclaste commented Sep 15, 2024
View reviewed changes
@nicoonoclaste nicoonoclaste changed the title lib.fetchers: add normalizeHashes wrapper lib.fetchers: add hash-normalization helpers Sep 15, 2024
@nicoonoclaste nicoonoclaste mentioned this pull request Sep 15, 2024
Open
54 tasks
@nicoonoclaste nicoonoclaste marked this pull request as ready for review September 15, 2024 14:24
@nicoonoclaste nicoonoclaste mentioned this pull request Sep 15, 2024
Closed
4 tasks
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 15, 2024

Also converted buildBazelPackage, to give an example of a fetcher using normalizeHashes

@nicoonoclaste nicoonoclaste mentioned this pull request Sep 15, 2024
Closed
6 tasks
Aleksanaa
Aleksanaa reviewed Sep 15, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

Looks reasonable; missing doc comments (or we don't write it for "internal use"?); Should evaluate performance impact after converting some major fetchers

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 15, 2024 •
edited
Loading

missing doc comments (or we don't write it for "internal use"?)

Good catch, I forgot to make a new documentation comment when splitting up the function

PS: The comments also do not yet comply with the new documentation conventions, though writing a type signature for those functions will be... fun 😓

Aleksanaa
Aleksanaa reviewed Sep 15, 2024
View reviewed changes
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 15, 2024

Should evaluate performance impact after converting some major fetchers

That was indeed my plan. If you think the PR is in a reasonable state, and once ofborg is done evaluating it, I can plonk the wrapper on fetchgit or such and see how ofborg's eval performance report looks like.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 15, 2024
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 15, 2024

Fixed the eval issue (forgot to update buildBazelPackage after changing the argument's name) and rebased to squash the fixup! commits

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 15, 2024

I can plonk the wrapper on fetchgit or such and see how ofborg's eval performance report looks like.

Done, nixpkgs-review is running, and let's see the performance impact as measured by ofborg.

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 15, 2024

Result of nixpkgs-review pr 342072 run on aarch64-darwin 1

1 package built:
  • nixpkgs-manual

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 15, 2024

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

1 package blacklisted:
  • nixos-install-tools
2 packages built:
  • disko
  • nixpkgs-manual

nicoonoclaste
nicoonoclaste commented Sep 15, 2024
View reviewed changes
@nicoonoclaste nicoonoclaste mentioned this pull request Sep 15, 2024
Merged
7 tasks
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 16, 2024

From the evaluation perf. report, this didn't have a significant impact on memory use, but may have made nixpkgs 4% slower to eval, though IDK what's the measurement noise on that metric.

There's a couple potential optimizations I can try, though.

Aleksanaa
Aleksanaa reviewed Sep 16, 2024
View reviewed changes
Aleksanaa
Aleksanaa reviewed Sep 16, 2024
View reviewed changes
@nicoonoclaste

This comment was marked as off-topic.

Sign in to view
@Aleksanaa
Copy link
Copy Markdown
Member

Aleksanaa commented Sep 17, 2024 •
edited
Loading

@Aleksanaa if this passes your review, I'll do one last no-diff rebase (as a treat) and this should be good to merge.

I'm sorry I can't give final review as I'm not really familiar with lib function guidelines. The only thing arise to my mind now is testing if hash = "" (assuming lib.fakeHash) and sha256 = "" (also assuming lib.fakeHash and would error out with SRI hash) still works as expected

(And I'm preparing for GRE test at Friday)

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024 •
edited
Loading

I'm sorry I can't give final review as I'm not really familiar with lib function guidelines.

No problem, and thanks for letting me know to look for another reviewer.

The only thing arise to my mind now is testing if hash = "" (assuming lib.fakeHash) and sha256 = "" (also assuming lib.fakeHash and would error out with SRI hash) still works as expected

👍
I serendipitously realised this was a concern while (re)reading the doc's chapter on fetchers, and was just writing a fix for it :D

And I'm preparing for GRE test at Friday

加油！

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024

Added a cleaner error message for cases like sha1 = "";

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024 •
edited
Loading

Rebased now, since this won't make final review harder, introducing no changes

@nicoonoclaste nicoonoclaste force-pushed the sha-to-SRI/fetchers branch 2 times, most recently from eb0d28b to f5e52f8 Compare September 17, 2024 08:29
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024

Reformated the tests

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024

Forgot to update some commit messages when renaming the functions; no change

@nicoonoclaste nicoonoclaste force-pushed the sha-to-SRI/fetchers branch 2 times, most recently from a11da5f to 4a02b5b Compare September 17, 2024 09:13
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024 •
edited
Loading

Fixed the way the new test file is invoked

PS: That took a bit of back-and-forth, but finally got it 🎉

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024 •
edited
Loading

Formated lib/tests/fetchers.nix (again) and squashed the fixup commits away

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024

Result of nixpkgs-review pr 342072 run on aarch64-darwin 1

1 package built:
  • nixpkgs-manual

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024

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

1 package blacklisted:
  • nixos-install-tools
2 packages built:
  • disko
  • nixpkgs-manual

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024

Result of nixpkgs-review pr 342072 run on aarch64-darwin 1

1 package built:
  • nixpkgs-manual

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Sep 17, 2024

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

1 package blacklisted:
  • nixos-install-tools
2 packages built:
  • disko
  • nixpkgs-manual

@nicoonoclaste nicoonoclaste merged commit 45b9542 into NixOS:master Sep 17, 2024
@nicoonoclaste nicoonoclaste deleted the sha-to-SRI/fetchers branch September 17, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkg20001 mkg20001 mkg20001 approved these changes

@philiptaron philiptaron Awaiting requested review from philiptaron

@infinisil infinisil Awaiting requested review from infinisil

@Aleksanaa Aleksanaa Awaiting requested review from Aleksanaa

Assignees

No one assigned

Labels

6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) 6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nicoonoclaste @fricklerhandwerk @Aleksanaa @mkg20001