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

treewide: remove all cargoSha256 usage #323983

Merged
merged 7 commits into from
Jul 4, 2024

Conversation

Aleksanaa
Copy link
Member

@Aleksanaa Aleksanaa commented Jul 2, 2024

Description of changes

First treewide (easy)

Treewide command:

find . -type f -name "*.nix" -print0 | xargs -0 sed -i 's/cargoSha256 = "sha256-/cargoHash = "sha256-/g'

Second treewide

Treewide bash script (which was executed in pkgs):

#!/usr/bin/env bash

process_line() {
    local filename=${1%:}
    if [[ $4 =~ \"(.*)\"\; ]]; then
      local sha256="${BASH_REMATCH[1]}"
    fi

    [[ -z $sha256 ]] && return 0

    local hash=$(nix hash to-sri --type sha256 $sha256)

    echo "Processing: $filename"
    echo "  $sha256 => $hash"

    sed -i "s|cargoSha256 = \"$sha256\"|cargoHash = \"$hash\"|" $filename
}

# split output by line
grep -r 'cargoSha256 = ' . | while IFS= read -r line; do
    # split them further by space
    read -r -a parts <<< "$line"
    process_line "${parts[@]}"
done

There are some exceptions, basically using a wrapped builder, or writing in let. I've manually fixed them. Would be better to check again.

buildRustPackage changes

I'm not really sure about this part.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@Aleksanaa
Copy link
Member Author

Logically speaking it should not cause such rebuilds (or actually it will)?

@Aleksanaa Aleksanaa marked this pull request as draft July 2, 2024 10:17
@JohnRTitor
Copy link
Contributor

Yeah, I am not sure if it's worth the rebuilding effort. Improvements like this can come atomically, like when updating that specific package.

@Aleksanaa
Copy link
Member Author

Aleksanaa commented Jul 2, 2024

Ah, I see. The hash here will not only be consumed by buildRustPackage, it also exists as an environment variable.

Improvements like this can come atomically, like when updating that specific package.

In theory we should deprecate it and avoid further usages altogether. We have already done that for golang's vendorHash (vendorSha256).

@h7x4
Copy link
Member

h7x4 commented Jul 2, 2024

Yeah, I am not sure if it's worth the rebuilding effort. Improvements like this can come atomically, like when updating that specific package.

I don't know about this. It feels like it's better to do a treewide into staging and let hydra take the hit once, rather than keep spending review time on it for random PRs. It will probably also make a higher percentage of new PRs use cargoHash, seeing how people often use another package as reference.

@Aleksanaa
Copy link
Member Author

Once we have all treewide changes done, we can let it throw a warning, and let cargoSha256 a history. I'm currently considering completing other changes at the same time.

@github-actions github-actions bot added the 6.topic: TeX Issues regarding texlive and TeX in general label Jul 3, 2024
@Aleksanaa Aleksanaa changed the title treewide: change cargoSha256 with SRI hash to cargoHash treewide: remove all cargoSha256 usage Jul 3, 2024
@Aleksanaa Aleksanaa force-pushed the cargoSha256-change branch 3 times, most recently from ea1c933 to bde993f Compare July 3, 2024 12:53
@Aleksanaa Aleksanaa changed the base branch from master to staging July 3, 2024 12:56
@Aleksanaa Aleksanaa marked this pull request as ready for review July 3, 2024 12:59
@Aleksanaa Aleksanaa requested a review from Mic92 July 3, 2024 13:01
@Mic92
Copy link
Member

Mic92 commented Jul 3, 2024

Yeah, I am not sure if it's worth the rebuilding effort. Improvements like this can come atomically, like when updating that specific package.

Most of the time it won't make a difference because something else in staging will have caused the rust toolchain to rebuild from source anyway.

@Aleksanaa
Copy link
Member Author

Oh the conflict is in staging, not master

@Aleksanaa Aleksanaa marked this pull request as draft July 3, 2024 13:31
This is done with the following bash script:

```
#!/usr/bin/env bash
process_line() {
    local filename=${1%:}
    if [[ $4 =~ \"(.*)\"\; ]]; then
      local sha256="${BASH_REMATCH[1]}"
    fi
    [[ -z $sha256 ]] && return 0
    local hash=$(nix hash to-sri --type sha256 $sha256)
    echo "Processing: $filename"
    echo "  $sha256 => $hash"
    sed -i "s|cargoSha256 = \"$sha256\"|cargoHash = \"$hash\"|"
$filename
}

# split output by line
grep -r 'cargoSha256 = ' . | while IFS= read -r line; do
    # split them further by space
    read -r -a parts <<< "$line"
    process_line "${parts[@]}"
done

```
Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

LGTM, passes my eyeball test

@JohnRTitor JohnRTitor merged commit 410d121 into NixOS:staging Jul 4, 2024
28 checks passed
wolfgangwalther added a commit to wolfgangwalther/nixpkgs that referenced this pull request Jul 8, 2024
This is a mismatch after merging both of:
- NixOS#323983
- NixOS#322749
@doronbehar
Copy link
Contributor

Missed rust-analyzer, fix is at:

#326491

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.

5 participants