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.

Sign up for GitHub

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

Jump to bottom

fetchpatch2: init #182250

Merged
merged 1 commit into from
Dec 1, 2022
Merged

fetchpatch2: init #182250

merged 1 commit into from
Dec 1, 2022

Conversation

Artturin
Copy link
Member

allows us to use the new features of patchutils without having to reset
all fetchpatch hashes in nixpkgs

#32084

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

@Artturin
fetchpatch2: init
4187709 
allows us to use the new features of patchutils without having to reset
all fetchpatch hashes in nixpkgs

NixOS#32084
@Artturin Artturin mentioned this pull request Jul 20, 2022
Open
@vcunat
Copy link
Member

vcunat commented Jul 20, 2022

In principle I like the approach. Before merging, we perhaps should think if there's some other potentially hash-changing... change that we might want to have as well.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Jul 20, 2022
@Artturin
Copy link
Member Author

In principle I like the approach. Before merging, we perhaps should think if there's some other potentially hash-changing... change that we might want to have as well.

I'm not aware of those and they might also have been implemented or fixed already because our current fetchpatch patchutils is from 2013

SuperSandro2000
SuperSandro2000 reviewed Jul 20, 2022
View reviewed changes
pkgs/test/default.nix
@@ -31,6 +31,7 @@ with pkgs;

fetchurl = callPackages ../build-support/fetchurl/tests.nix { };
fetchpatch = callPackages ../build-support/fetchpatch/tests.nix { };
fetchpatch2 = callPackages ../build-support/fetchpatch/tests.nix { fetchpatch = fetchpatch2; };
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this right into the test then we could also reduce the changes in all-packages.

Copy link
Member

Choose a reason for hiding this comment

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

I think the current solution is decent. If we need something else, we can change it.

@roberth
Copy link
Member

roberth commented Jul 21, 2022

In principle I like the approach. Before merging, we perhaps should think if there's some other potentially hash-changing... change that we might want to have as well.

#48567 is the only issue I've found, but can be largely resolved by hashing parameters into a name prefix or suffix. That's not a content hash changing operation though, so there's no benefit in bundling that one.

roberth
roberth approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

LGTM, but needs more eyes.

@mweinelt mweinelt added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 22, 2022
@IvarWithoutBones IvarWithoutBones added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jul 23, 2022
SuperSandro2000
SuperSandro2000 approved these changes Nov 30, 2022
View reviewed changes
pkgs/top-level/all-packages.nix
patchutils = buildPackages.patchutils_0_3_3;
} // {
tests = pkgs.tests.fetchpatch;
version = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Since fetchurl now support pname+version we could set this via that but not super important.

Copy link
Member

Choose a reason for hiding this comment

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

That'd be on the result of fetchpatch, whereas the test needs it to have a version for the function itself.
I think having the version on the result (derivation) would be confusing. (Why does my patch have a version I didn't specify?)

@roberth roberth merged commit c2f071a into NixOS:master Dec 1, 2022
@github-actions github-actions bot mentioned this pull request Dec 1, 2022
Merged
1 task
@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

Successfully created backport PR #203847 for release-22.11.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

Git push to origin failed for release-22.11 with exitcode 1

@Artturin
Copy link
Member Author

Artturin commented Dec 4, 2022

perhaps we should do the the downloading in a fod and then the patchutils stuff in a non fod to prevent hash incompabilities when we update patchutils

@Artturin Artturin deleted the fetchpatch2 branch December 4, 2022 13:57
@jtojnar
Copy link
Member

jtojnar commented Dec 4, 2022

The main point of fetchpatch is making FOD for generated patches that can change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@IvarWithoutBones IvarWithoutBones IvarWithoutBones approved these changes

@ncfavier ncfavier Awaiting requested review from ncfavier

Assignees
No one assigned
Labels
6.topic: fetch 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Artturin @vcunat @roberth @jtojnar @SuperSandro2000 @IvarWithoutBones @mweinelt