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 refs/tags/ from github release meta.changelog urls #338301

Merged

Conversation

pbsds
Copy link
Member

@pbsds pbsds commented Aug 30, 2024

diff of jq <packages.json 'to_entries[]|"\(.key) \(.value.meta.changelog)"' -r:

https://gist.github.com/pbsds/49b2e2ae5c9b967a0972bbc3c2597c12

Description of changes

the majority of this pr was made using:

#!/usr/bin/env nix-shell
#!nix-shell --pure -i bash -p ripgrep sd jq git lix

export NIXPKGS_ALLOW_UNFREE=1
export NIXPKGS_ALLOW_INSECURE=1
export NIXPKGS_ALLOW_BROKEN=1

git restore .

test -s packages.json || ( set -x;
  time nix-env --extra-experimental-features no-url-literals --option system x86_64-linux -f ./. -qaP --json --meta --show-trace --no-allow-import-from-derivation > packages.json
)

jq <packages.json 'to_entries[]
        | select(.value.meta.position==null|not)
        | select(.value.meta.changelog==null|not)
        | "\(.key)\t\(.value.meta.changelog)\t\(.value.meta.position)"' -r |
    sed -e "s#\t$(realpath .)/#\t#" |
    sed -e 's#:\([0-9]*\)$#\t\1#' |
    grep -E '/releases/tag/refs/tags/' |
    while read attrpath changelog fname col; do
        echo "$attrpath"
        (set -x
            sd -F '/releases/tag/${src.rev}' '/releases/tag/${lib.removePrefix "refs/tags/" src.rev}' "$fname"
            sd -F '/releases/tag/${finalAttrs.src.rev}' '/releases/tag/${lib.removePrefix "refs/tags/" finalAttrs.src.rev}' "$fname"
            sd -F '/releases/tag/${self.src.rev}' '/releases/tag/${lib.removePrefix "refs/tags/" self.src.rev}' "$fname"
        )
    done

( set -x;
  time nix-env --extra-experimental-features no-url-literals --option system x86_64-linux -f ./. -qaP --json --meta --show-trace --no-allow-import-from-derivation > packages2.json
)

diff -u \
  <(jq <packages.json  'to_entries[]|"\(.key) \(.value.meta.changelog)"' -r | sort) \
  <(jq <packages2.json 'to_entries[]|"\(.key) \(.value.meta.changelog)"' -r | sort) | tee diff.patch

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.

@github-actions github-actions bot added 6.topic: python 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab labels Aug 30, 2024
@pbsds pbsds marked this pull request as ready for review August 30, 2024 02:49
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Wish we had an API for constructing structured GitHub links, but LGTM.

I still wish we had any idea whether anyone is looking at these URLs ever. The fact that this many were broken and no user noticed suggests no.

@pbsds
Copy link
Member Author

pbsds commented Aug 30, 2024

I check them quite often, it should be encouraged rather than discouraged IMO

@ofborg ofborg bot added 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 Aug 30, 2024
@pbsds pbsds requested a review from philiptaron August 30, 2024 03:23
@philiptaron
Copy link
Contributor

Thank you for making your script public in this PR and others, Peder (@pbsds). I learn so much from these "worked demonstrations". It's really appreciated.

@zimbatm zimbatm mentioned this pull request Aug 30, 2024
13 tasks
@zimbatm
Copy link
Member

zimbatm commented Aug 30, 2024

Since they are computed values, I wonder if they couldn't be automated away in 90% of the cases. Not a blocker for this PR, but more as a follow-up conversation (see #338366).

@philiptaron
Copy link
Contributor

Hmmm. I wonder if we could abstract this with a tag argument to fetchFromGitHub which understands refs/tags. That would eliminate both the refs/tags in the src and also the refs/tags removal in the changelog entries.

It would be mutually exclusive with rev, since it's another way of specifying it.

@emilazy
Copy link
Member

emilazy commented Aug 30, 2024

I strongly support a tag argument regardless of anything to do with changelogs.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 31, 2024
Copy link
Member

@natsukium natsukium left a comment

Choose a reason for hiding this comment

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

I think it used to work fine, but now it does not.
Thanks for your work.

@natsukium natsukium merged commit c90f794 into NixOS:master Sep 1, 2024
30 checks passed
@dotlambda
Copy link
Member

dotlambda commented Oct 4, 2024

What's the point of this change? Is making the URLs a little nicer worth it?
EDIT: Sorry, I should have read the actual script. It doesn't change uses of blob/${src.rev} so this is an actual improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: python 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 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants