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:

GitHub link to the corresponding issue

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 mjo

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).

comment:2 Changed 2 years ago by mkoeppe

Milestone: sage-9.3sage-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 dimpase

Authors: Dima Pasechnik
Branch: u/dimpase/config/acceptposixpatch
Commit: e0fcafc87a954a92a0715685f84f05f56bc91aa6
Dependencies: #30668

need to check whether an oldish macOS patch (GNU patch 2.5.8, it says) is OK.


New commits:

9bfbc41update for autoconf 2.71
2791d92Merge branch 'u/dimpase/ticket/30668' of trac.sagemath.org:sage into u/dimpase/config/acceptposixpatch
e0fcafcaccept any POSIX-compatible patch(1)
Last edited 20 months ago by slelievre (previous) (diff)

comment:4 Changed 20 months ago by mjo

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.

  1. Find "patch" with AC_PATH_PROG
  2. Try printf "" | ${PATCH} --posix. If it works, set SAGE_PATCH_ARGS="--posix".
  3. Rename build/bin/sage-apply-patches with an .in suffix and add it to AC_CONFIG_FILES
  4. Set patch_args="@SAGE_PATCH_ARGS@" in build/bin/sage-apply-patches.in.

That way, if --posix is supported, we use it. Otherwise, we assume POSIX conformance.

comment:5 Changed 20 months ago by 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.

comment:6 Changed 20 months ago by dimpase

on the other hand I see no harm in just dropping --no-backup-if-mismatch as it doesn't buy us anything.

comment:7 in reply to:  5 ; Changed 20 months ago by 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.

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 Changed 20 months ago by mkoeppe

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 in reply to:  7 Changed 20 months ago by dimpase

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 in reply to:  8 Changed 20 months ago by dimpase

Replying to mkoeppe:

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?

I don't think it's so hard, it's just an executable after all. And as a bonus we can just get rid of Sage's patch spkg, after this is done.

comment:11 Changed 20 months ago by mkoeppe

There are no problems with Sage's patch spkg, please don't create new problems

comment:12 Changed 20 months ago by mkoeppe

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 in reply to:  11 Changed 20 months ago by dimpase

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:14 Changed 20 months ago by mkoeppe

comment 11 covers this.

comment:15 in reply to:  14 Changed 20 months ago by dimpase

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 mkoeppe

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 dimpase

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 mkoeppe

Milestone: sage-9.4sage-9.5

comment:19 Changed 14 months ago by mkoeppe

Milestone: sage-9.5sage-9.6

comment:20 Changed 10 months ago by mkoeppe

Milestone: sage-9.6sage-9.7

comment:21 Changed 5 months ago by mkoeppe

Milestone: sage-9.7sage-9.8

comment:22 Changed 10 days ago by mkoeppe

Milestone: sage-9.8
Note: See TracTickets for help on using tickets.