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

Replace the dependency on GNU patch by a strict dependency on git #5400

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

kit-ty-kate
Copy link
Member

Fixes #3433
Fixes #3782
Fixes #3639 for good
Helps #3482

While looking at #3639 happening for many macOS users, I realized that git apply basically ships with its own version of GNU patch which works correctly.

The original issue is that macOS is a POSIX-complient OS. However POSIX-complient patch seems to be a nightmare: you can't tell it to delete files. It'll just make it empty instead. There are non-posix options (-E) that deletes empty files anyway but those have another problem where it can't distinguish between you deleting the file and emptying the file... This issue also appears on BSDs.

The solution here is to make git a required dependency. Most users will have it anyway and many features are missing without it so it's not too much of a stretch to make it mandatory.

@dra27
Copy link
Member

dra27 commented Dec 22, 2022

I have a recollection that this wasn't a silver bullet on the Windows side but I think that may have been before the patch rewriter was added. Regardless, this is well-worth revising, thanks! Even if we end up having to keep the patch pre-processing, at least there's only one patch program then to be considering...

@MisterDA
Copy link
Contributor

MisterDA commented Feb 7, 2023

This is great, I've had problems with GNU patch on Windows from Cygwin, which lead me to replace this code

cd ~/opam-repository \
&& (git cat-file -e $OPAM_HASH || git fetch origin master) \
&& git reset -q --hard $OPAM_HASH \
&& git --no-pager log --no-decorate -n1 --oneline \
&& opam update -u

with

cd /home/opam/opam-repository \
&& (git cat-file -e $OPAM_HASH || git fetch origin opam2) \
&& git reset -q --hard $OPAM_HASH \
&& git --no-pager log --no-decorate -n1 --oneline \
&& rsync -ar --update --exclude='.git' ./ /cygdrive/c/opam/.opam/repo/default \
&& ocaml-env exec --64 -- opam update -u

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

It needs also an update of the documentation, see patches: section.
On the code/change itself, lgtm!

master_changes.md Show resolved Hide resolved
@kit-ty-kate
Copy link
Member Author

done

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

LGTM - if whatever it was in the past that caused me to revert this idea re-emerges, we'll be able to address it during the rest of the release cycle.

@kit-ty-kate kit-ty-kate merged commit 321ca42 into ocaml:master Mar 4, 2024
29 checks passed
@kit-ty-kate kit-ty-kate deleted the no-gpatch branch March 4, 2024 18:14
@kit-ty-kate kit-ty-kate mentioned this pull request Mar 4, 2024
4 tasks
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this pull request Mar 20, 2024
This reverts commit 321ca42, reversing
changes made to a4a651a.
kit-ty-kate added a commit to kit-ty-kate/opam that referenced this pull request Mar 20, 2024
This reverts commit 321ca42, reversing
changes made to a4a651a.
@rjbou
Copy link
Collaborator

rjbou commented Mar 20, 2024

Reverted in #5891, see comment for explanation

kit-ty-kate added a commit that referenced this pull request Mar 20, 2024
Revert "Merge pull request #5400 from kit-ty-kate/no-gpatch"
@rjbou rjbou modified the milestones: 2.2.0~beta2, 2.1.6 May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants