Opened 2 years ago
Last modified 10 days ago
#30481 new enhancement
allow any posix-compliant PATCH(1)
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | |
Component: | build: configure | Keywords: | |
Cc: | gh-thierry-FreeBSD, mjo | Merged in: | |
Authors: | Dima Pasechnik | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | u/dimpase/config/acceptposixpatch (Commits, GitHub, GitLab) | Commit: | e0fcafc87a954a92a0715685f84f05f56bc91aa6 |
Dependencies: | #30668 | Stopgaps: |
Description
currently we require GNU patch, version at least 2.7 (from 2009).
In particular, BSD patch does not know about --no-backup-if-mismatch
, something that Sage used. An equivalent, and known to BSD patch as well as GNU patch, option is --posix
.
So we could use --posix
and change spkg-configure to accept BSD patch too.
I am not sure what's needed to be checked, besides that --posix
is accepted. Something to learn from https://savannah.gnu.org/forum/forum.php?forum_id=7361 ?
Change History (22)
comment:1 Changed 2 years ago by
comment:2 Changed 2 years ago by
Milestone: | sage-9.3 → sage-9.4 |
---|
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
comment:3 Changed 20 months ago by
Authors: | → Dima Pasechnik |
---|---|
Branch: | → u/dimpase/config/acceptposixpatch |
Commit: | → e0fcafc87a954a92a0715685f84f05f56bc91aa6 |
Dependencies: | → #30668 |
comment:4 Changed 20 months ago by
This will actually only find a non-POSIX patch, since the --posix
option only exists in non-conforming implementations =)
I think printf "" | patch --posix
might be a more reliable test of support for the --posix
flag. And if --posix
is not supported, we should assume that "patch" conforms, i.e.
- Find "patch" with AC_PATH_PROG
- Try
printf "" | ${PATCH} --posix
. If it works, setSAGE_PATCH_ARGS="--posix"
. - Rename
build/bin/sage-apply-patches
with an.in
suffix and add it toAC_CONFIG_FILES
- Set
patch_args="@SAGE_PATCH_ARGS@"
inbuild/bin/sage-apply-patches.in
.
That way, if --posix
is supported, we use it. Otherwise, we assume POSIX conformance.
comment:5 follow-up: 7 Changed 20 months ago by
meanwhile I am finding out that we cannot call patch
with --posix
option, as it doesn't allow file creation.
So the patch in psutil spkg does not apply.
comment:6 Changed 20 months ago by
on the other hand I see no harm in just dropping --no-backup-if-mismatch
as it doesn't buy us anything.
comment:7 follow-up: 9 Changed 20 months ago by
Replying to dimpase:
meanwhile I am finding out that we cannot call
patch
with--posix
option, as it doesn't allow file creation. So the patch in psutil spkg does not apply.
What's the problem with file creation?
The --posix
docs for GNU patch mention that it will become impossible to remove files (you can only empty them), but don't say anything about file creation.
It may be important: if we start to accept any "patch", then we have to be prepared for one that follows the POSIX behavior. In other words, if the patches we ship only work with GNU/BSD patch, then we should probably check for one of those and not for any POSIX-compatible patch. (In that case, we can still test for the backup flag with printf "" | patch --no-backup-if-mismatch
)
comment:8 follow-up: 10 Changed 20 months ago by
Relaxing the spkg-configure.m4 of patch sounds to me like asking for trouble. Recall the insanity that had to be worked around in #30403?
comment:9 Changed 20 months ago by
Replying to mjo:
Replying to dimpase:
meanwhile I am finding out that we cannot call
patch
with--posix
option, as it doesn't allow file creation. So the patch in psutil spkg does not apply.What's the problem with file creation?
The
--posix
docs for GNU patch mention that it will become impossible to remove files (you can only empty them), but don't say anything about file creation.
I suppose it's due to the way patch --posix
determines the filename to patch, and gathers it's /dev/null
, oops.
(this is what GNU patch
does, I didn't look at what's BSD patch
does.)
It may be important: if we start to accept any "patch", then we have to be prepared for one that follows the POSIX behavior. In other words, if the patches we ship only work with GNU/BSD patch, then we should probably check for one of those and not for any POSIX-compatible patch. (In that case, we can still test for the backup flag with
printf "" | patch --no-backup-if-mismatch
)
I agree, some more testing is needed.
comment:10 Changed 20 months ago by
comment:11 follow-up: 13 Changed 20 months ago by
There are no problems with Sage's patch spkg, please don't create new problems
comment:12 Changed 20 months ago by
And until someone is sufficiently invested in a *BSD port to actually set up automatic testing, as discussed several times, let's please not use any BSD platform as motivation for changes like this.
comment:13 Changed 20 months ago by
Replying to mkoeppe:
There are no problems with Sage's patch spkg, please don't create new problems
my primary issue with patch is that it's part of the Sage's toolchain, and as such 1st in line to be kicked out.
comment:15 Changed 20 months ago by
Replying to mkoeppe:
comment 11 covers this.
How? I don't see why we need to keep toolchain components vendored.
comment:16 Changed 20 months ago by
We are not vendoring it. We are providing a fallback installation script for the benefit of users who don't have it on their machines. It is working well: It installs correctly and has not caused other maintenance issues. (That's in contrast to the problematic gcc
package - which is a fallback that works sometimes but not always; and requires relatively frequent upgrades.) So, let's please not solve nonexisting problems.
comment:17 Changed 20 months ago by
so far one outcome is that --no-backup-if-mismatch
may be just dropped (this option is just cosmetic), and in fact native macOS patch
, which is GNU patch 2.5.8, may be used just as well.
I gather that the fallback is simply not needed, just as it's not needed for tar
(tar
used to be a Sage spkg).
Whether BSD patch should be allowed is another question.
Oh, and iml spkg has a patch for a non-existing file, something I found out using --posix
option :-)
comment:18 Changed 19 months ago by
Milestone: | sage-9.4 → sage-9.5 |
---|
comment:19 Changed 14 months ago by
Milestone: | sage-9.5 → sage-9.6 |
---|
comment:20 Changed 10 months ago by
Milestone: | sage-9.6 → sage-9.7 |
---|
comment:21 Changed 5 months ago by
Milestone: | sage-9.7 → sage-9.8 |
---|
comment:22 Changed 10 days ago by
Milestone: | sage-9.8 |
---|
Sounds good to me. Additionally we could check to see if the system
patch
supports a--posix
flag. If it does, we should pass it; otherwise we may simply assume that the implementation respects POSIX (and does not create backups).