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

stdenv: Allow changing the patch command #251622

Closed
wants to merge 1 commit into from

Conversation

Tom-Hubrecht
Copy link
Contributor

@Tom-Hubrecht Tom-Hubrecht commented Aug 26, 2023

Description of changes

The patch utility is not fully compatible with the diffs generated by
git, one missing feature is the renaming of files. As GitHub exports diffs
of a PR with renames, it means that those patches are not guaranteed to work
with patch.

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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Aug 26, 2023
@tobim
Copy link
Contributor

tobim commented Aug 26, 2023

The commit message should provide a motivation and this addition needs to be documented.

@Tom-Hubrecht
Copy link
Contributor Author

The commit message should provide a motivation and this addition needs to be documented.

It is now fixed.

@tobim
Copy link
Contributor

tobim commented Aug 26, 2023

The patch utility is not fully compatible with the diffs generated by git, one missing feature is the renaming of files.

If this was the case a lot of patches in the tree would not apply. Can you provide a source for this?

@Tom-Hubrecht
Copy link
Contributor Author

If this was the case a lot of patches in the tree would not apply. Can you provide a source for this?

Sure, the PR that made me realize this was #250565 , when trying to apply https://github.com/NixOS/nixpkgs/pull/250565.patch , the file gatherer-Cargo.lock would not be created.

@Tom-Hubrecht Tom-Hubrecht changed the base branch from master to staging August 26, 2023 18:54
The `patch` utility is not fully compatible with the diffs generated by
`git`, one missing feature is the renaming of files. As GitHub exports diffs
of a PR with renames, it means that those patches are not guaranteed to work
with `patch`.
@tobim
Copy link
Contributor

tobim commented Aug 26, 2023

Sure, the PR that made me realize this was #250565 , when trying to apply https://github.com/NixOS/nixpkgs/pull/250565.patch , the file gatherer-Cargo.lock would not be created.

Why should a patch for nixpkgs itself be applied in stdenv?
The patch linked to applies fine for me:

> curl https://patch-diff.githubusercontent.com/raw/NixOS/nixpkgs/pull/250565.patch | 
patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 37372    0 37372    0     0  92274      0 --:--:-- --:--:-- --:--:-- 92276
patching file pkgs/applications/misc/mission-center/Cargo.lock
patching file pkgs/applications/misc/mission-center/default.nix
patching file pkgs/applications/misc/mission-center/gatherer-Cargo.lock (renamed from pkgs/applications/misc/mission-center/proxy-Cargo.lock)
> ls pkgs/applications/misc/mission-center/gatherer-Cargo.lock                        
pkgs/applications/misc/mission-center/gatherer-Cargo.lock

Is it possible that the patch in your PATH is not GNU patch?

@Tom-Hubrecht
Copy link
Contributor Author

Sure, the PR that made me realize this was #250565 , when trying to apply 250565.patch , the file gatherer-Cargo.lock would not be created.

Why should a patch for nixpkgs itself be applied in stdenv? The patch linked to applies fine for me:

> curl https://patch-diff.githubusercontent.com/raw/NixOS/nixpkgs/pull/250565.patch | 
patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 37372    0 37372    0     0  92274      0 --:--:-- --:--:-- --:--:-- 92276
patching file pkgs/applications/misc/mission-center/Cargo.lock
patching file pkgs/applications/misc/mission-center/default.nix
patching file pkgs/applications/misc/mission-center/gatherer-Cargo.lock (renamed from pkgs/applications/misc/mission-center/proxy-Cargo.lock)
> ls pkgs/applications/misc/mission-center/gatherer-Cargo.lock                        
pkgs/applications/misc/mission-center/gatherer-Cargo.lock

Is it possible that the patch in your PATH is not GNU patch?

Oh, I found the issue, the original patch applies fine for me as well if I get it with curl, (I do have gnupatch in the path), the real problem comes from pkgs.fetchpatch which removes the mentions of the renames, so this PR wouldn't help at all.

@tobim
Copy link
Contributor

tobim commented Aug 26, 2023

Ah, that is tracked in #32084. You can try to use fetchpatch2.

@Tom-Hubrecht Tom-Hubrecht deleted the patchCommand branch May 5, 2024 14:37
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.

None yet

2 participants