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

yarn2nix: use sha512 by default #149834

Closed
wants to merge 11 commits into from
Closed

Conversation

prusnak
Copy link
Member

@prusnak prusnak commented Dec 9, 2021

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, 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from Ma27 December 9, 2021 13:03
@prusnak prusnak marked this pull request as ready for review December 9, 2021 13:04
Comment on lines +62 to 63
resolved: `${url}#${newSha512}`,
}
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure this is supported. At least I have not seen it anywhere.

It would probably be better to use integrity: `sha512-${newSha512}`

@@ -34,7 +34,7 @@ function getSha1(url) {
}

// Object -> Object
async function fixPkgAddMissingSha1(pkg) {
async function fixPkgAddMissingSha512(pkg) {
// local dependency

Copy link
Member

Choose a reason for hiding this comment

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

I would also add something like

if (pkg.integrity) {
  return pkg;
}

as according to my understanding, integrity has priority over the hash in resolved.

@@ -46,21 +46,21 @@ async function fixPkgAddMissingSha1(pkg) {
return pkg
}

const [url, sha1] = pkg.resolved.split('#', 2)
const [url, sha512] = pkg.resolved.split('#', 2)
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned, I am not sure if resolved field can contain anything other than sha1. Will prolly need to scour https://github.com/yarnpkg/yarn


if (sha1 || url.startsWith('https://codeload.github.com')) {
if (sha512 || url.startsWith('https://codeload.github.com')) {
return pkg
Copy link
Member

Choose a reason for hiding this comment

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

We should probably do getSha512 if the sha1 is present.

};
}
{
name = "to_readable_stream___to_readable_stream_1.0.0.tgz";
path = fetchurl {
name = "to_readable_stream___to_readable_stream_1.0.0.tgz";
url = "https://registry.yarnpkg.com/to-readable-stream/-/to-readable-stream-1.0.0.tgz";
sha1 = "ce0aa0c2f3df6adf852efb404a783e77c0475771";
sha512 = "Iq25XBt6zD5npPhlLVXGFN3/gyR2/qODcKNNyTMd4vbm39HUaOiAM4PMq0eMVC/Tkxz+Zjdsc55g9yyz+Yq00Q==";
};
}
{
name = "to_regex_range___to_regex_range_2.1.1.tgz";
path = fetchurl {
name = "to_regex_range___to_regex_range_2.1.1.tgz";
url = "https://registry.yarnpkg.com/to-regex-range/-/to-regex-range-2.
Copy link
Member

Choose a reason for hiding this comment

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

Example of sha1

@yu-re-ka
Copy link
Contributor

The resolved field always uses sha1, so some parts of this change don't make sense. With #119522 we should already use sha512 most of the time?

Please also consider and test the IFD-mode of yarn2nix where we have to use what's in the yarn.lock file.

@prusnak
Copy link
Member Author

prusnak commented Dec 10, 2021

@jtojnar @yu-re-ka Feel free to take over this PR, unfortunately, I won't be able to pursue this further in the following days.

@prusnak prusnak marked this pull request as draft December 10, 2021 08:16
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 12, 2022
@prusnak prusnak closed this Jan 29, 2023
@prusnak prusnak deleted the yarn2nix-sha512 branch January 29, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: python 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants