we should not build patch on Cygwin on Windows 7

[dimpase]

On Windows 7 an executable named patch.exe will not run (UAC prevents this), unless
it is accompanied by patch.exe.manifest.
See
http://cygwin.com/ml/cygwin/2009-03/msg00010.html

So we just should not build it there, and use the Cygwin's patch instead.
I hope Cygwin's native 2.5.8 patch is good enough.

The modified patch spkg is here:

http://boxen.math.washington.edu/home/jdemeyer/spkg/patch-2.5.9.p2.spkg

(tested on Linux and on Win 7 with and without patch installed)

CC: @sagetrac-drkirkby

Component: porting: Cygwin

Author: Dima Pasechnik

Reviewer: David Kirkby, Karl-Dieter Crisman

Merged: sage-4.7.2.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/11232

[sagetrac-drkirkby]

Reviewer: David Kirkby

[sagetrac-drkirkby]

comment:3

The test is not working. I changed it a little, so it worked on Solaris instead of Cygwin. My changes are:

if [ "x$UNAME" = xSunOS ] ; then
   if ! patch --version | grep >/dev/null 'patch '; then
      echo "On Cygwin you must have patch installed"
      echo "via the Cygwin setup.exe program"
      exit 1
   fi
   exit 0
fi

then I created a script called 'patch', which just echos "foo"

drkirkby@laptop:~/patch-2.5.9.p1$ patch
foo

Now when I run spkg-install, it does not detect I don't have a usable 'patch; command:

drkirkby@laptop:~/patch-2.5.9.p1$ ./spkg-install
SAGE_LOCAL undefined ... exiting
Maybe run 'sage -sh'?
drkirkby@laptop:~/patch-2.5.9.p1$ 

There's no message I need to install patch.

I'm not sure exactly what you are trying to achieve here - this is not the way I would write a test. But the exit code of grep should be 0 in the case of a match and non-zero for the case of no match or an error.

I would also suggest putting the test to see if 'SAGE_ROOT' is set at the top, as it is with every other package.

A better place to check for the existence of 'patch' is actually in the prereq script, as we should let users know as soon as possible if their system needs extra things installed.

Dave

[dimpase]

comment:4

Replying to @sagetrac-drkirkby:

OK, I've updated the spkg-install in the spkg (same location) to do the test on the existence of SAGE_ROOT first.

The logic of the CYGWIN-specific test is that it runs 'patch --version' and exits with 1 if the output
does not contain 'patch '; otherwise, it exits with 0.
This test is shamelessly stolen from the bottom of spkg-install, where it tests for the version (I only removed the version part).

[dimpase]

comment:5

Replying to @dimpase:

Replying to @sagetrac-drkirkby:

reasons for actually trying to run patch, not only testing for existence, are obvious --- it tests more (especially given that nasty UAC "manifest" stuff). I did not put it into prereq, as other packages like this (e.g. Atlas) do it this way, too, so I merely conform to what is already done.

[sagetrac-drkirkby]

comment:6

If Cygwin needs patch installed this should be checked early on. We test that a Fortran compiler exists well before we start to use it. Why should patch be any different?

Exiting on Cygwin is sensible if there' no need to install patch, but testing if the program exists should in my opinion be done much earlier on. I believe the way to do that is probably to use AC_CHECK_PROG in the 'prereq' part of Sage.

Also note running patch with no arguments will leave it there sitting for input.

Would it not be better to install patch.exe.manifest from

http://cygwin.com/ml/cygwin/2009-03/msg00010.html

? Then we could install patch the same way as any other program, and know it behaves the same, as we have the same version.

Something along the lines of

if [ "x$UNAME" = xCYGWIN ] ; then
  cp patches/patch.exe.manifest "$SAGE_LOCAL/bin"
fi

Can you attach a Mercurial patch for review purposes, so we can see what you are trying to do. The ticket is much more informative if it has the changes attached.

Dave

[sagetrac-drkirkby]

comment:7

BTW,

http://cygwin.com/ml/cygwin/2009-03/msg00034.html

says "The latest patch package 2.5.8-9 comes already with a manifest file."

It seems to me the method proposed here is far from optimal.

Dave

[dimpase]

comment:8

Replying to @sagetrac-drkirkby:

BTW,

http://cygwin.com/ml/cygwin/2009-03/msg00034.html

says "The latest patch package 2.5.8-9 comes already with a manifest file."

It seems to me the method proposed here is far from optimal.

I wasted few hours yesterday trying this path. It is NOT merely copying an extra file somewhere. One has to use a Microsoft installer to cook up this "security exception", or an equivalent convoluted procedure requiring editing the registry and god knows what else (reading MS documentation on this stuff makes your head spin...)

Dave

[dimpase]

comment:9

Replying to @sagetrac-drkirkby:

If Cygwin needs patch installed this should be checked early on. We test that a Fortran compiler exists well before we start to use it. Why should patch be any different?

why should patch be different from Atlas? I went the way it was done for Atlas.
OK, there is an inconsistency in design here, as you point out,
but it's a minor and fixable one---if one has plenty of time and a Windows machine at hand to test things. But the gain would be small.

Exiting on Cygwin is sensible if there' no need to install patch, but testing if the program exists should in my opinion be done much earlier on. I believe the way to do that is probably to use AC_CHECK_PROG in the 'prereq' part of Sage.

Also note running patch with no arguments will leave it there sitting for input.

well, yes, it would. Hence there is an argument...

Can you attach a Mercurial patch for review purposes, so we can see what you are trying to do. The ticket is much more informative if it has the changes attached.

if you untar the spkg file and do

hg diff -r3:6

you will see the difference. Do we need to post perfectly reproducible things like this here?

[sagetrac-drkirkby]

comment:10

Replying to @dimpase:

Replying to @sagetrac-drkirkby:

If Cygwin needs patch installed this should be checked early on. We test that a Fortran compiler exists well before we start to use it. Why should patch be any different?

why should patch be different from Atlas?

Because the ATLAS method is incorrect. That's a good enough reason for me. Two wrongs don't make a right.

The ATLAS spkg-install happens to be very poor in many respects. It is a very small Python script that calls a large bash script. I don't think we should use ATLAS as a model.

I went the way it was done for Atlas.

Numerous tests for programs are performed - gcc, g++, gfortran, m4, bash. There's a test for the maths library too. All these checks for prerequisites are done at the start. Just because one package happens to do it the wrong way, does not mean we should copy that.

OK, there is an inconsistency in design here, as you point out,
but it's a minor and fixable one---if one has plenty of time and a Windows machine at hand to test things. But the gain would be small.

If it's minor and fixable is should be fixed. I don't think its a good idea to let a build progress any further than necessary if it is obvious it is going to fail.

Exiting on Cygwin is sensible if there' no need to install patch, but testing if the program exists should in my opinion be done much earlier on. I believe the way to do that is probably to use AC_CHECK_PROG in the 'prereq' part of Sage.

Also note running patch with no arguments will leave it there sitting for input.

well, yes, it would. Hence there is an argument...

Can you attach a Mercurial patch for review purposes, so we can see what you are trying to do. The ticket is much more informative if it has the changes attached.

if you untar the spkg file and do

hg diff -r3:6

you will see the difference. Do we need to post perfectly reproducible things like this here?

Well 99% of people do attach patches, which makes review a lot easier. It also leaves a record for others to see what is proposed and why specific solutions were rejected.

Anyway, I'm not happy with this approach to solving an issue with patch, when it's clear there are better ways that don't copy a method that's clearly flawed. I'm not going to put it back to "needs_work" again, as clearly you feel differently. You might find another reviewer who is happy with this method.

Dave

[dimpase]

comment:11

Replying to @sagetrac-drkirkby:
One reason to have this patch in anyway is that with the current build system it is perfectly possible to install the patch spkg after the build is done (sage-spkg does not check prereqs). And the absence of the current patch to the patch spkg would create a problem, as the Sage's patch will break.

This is a general weakness of the Sage build system, that also manifests elsewhere (e.g. it's possible to nuke any standard package this way, if there is an optional package of the same name, e.g. GLPK is an example).
But this needs another ticket.

[sagetrac-drkirkby]

comment:12

Replying to @dimpase:

Replying to @sagetrac-drkirkby:
One reason to have this patch in anyway is that with the current build system it is perfectly possible to install the patch spkg after the build is done (sage-spkg does not check prereqs). And the absence of the current patch to the patch spkg would create a problem, as the Sage's patch will break.

Yes, it is possible to do that. But normally one would type "make" and let it do the work. If you manually install items in an abnormal order, you should either know what you are doing or expect a failed installation.

This is a general weakness of the Sage build system, that also manifests elsewhere (e.g. it's possible to nuke any standard package this way, if there is an optional package of the same name, e.g. GLPK is an example).
But this needs another ticket.

Well, it's just crazy we have an old optional package with the same name is a current one in Sage. But that's an entirely different issue. I have created a ticket for that - #11247.

If someone wants to screw up their Sage build it is not hard (I've done it myself), but we should try to prevent it as much as possible. Hence checking they have the right perquisites should be done at the start of the build process.

[jhpalmieri]

comment:13

I want to point out that the patch spkg is a prerequisite of almost every other spkg, so practically, anything in its spkg-install script will happen very early in the installation procedure. I think that if we are requiring "patch" to be installed on cygwin, then it should be mentioned in the Cygwin section of the installation guide (#11237).

[sagetrac-drkirkby]

comment:14

One reasonable option is to check if the file '/usr/bin/patch' exists on Cygwin. Checking if the file exists does not need it to be executed, so I assume that Windows 7 will not object.

Although I've not tested this, adding something like these 4 lines

if [ "x`uname | sed 's/WIN.\+/WIN/'`" = xCYGWIN ] && [ ! -f /usr/bin/patch ] ; then
   echo "On Cygwin one must 'patch' on Cygwin fist. Do it by ..."
   exit 1
fi

to the script $SAGE_ROOT/spkg/base/prereq-0.8-install would allow one to check if patch is installed early on, before we start building .spkg's.

The steps to that would be something like:

• Add the above 4 lines to $SAGE_ROOT/spkg/base/prereq-0.8-install
• Change the 10^th line of $SAGE_ROOT/spkg/base/prereq-0.8-install to be 'TARGET=prereq-0.9'
• Rename $SAGE_ROOT/spkg/base/prereq-0.8-install to spkg/base/prereq-0.9-install
• extract the tar file prereq-0.8.tar
• rename the directory {{$SAGE_ROOT/spkg/base/prereq-0.8}}} to {{$SAGE_ROOT/spkg/base/prereq-0.9}}}
• Create the tar file {{$SAGE_ROOT/spkg/base/prereq-0.9.tar}}} by just taring up {{$SAGE_ROOT/spkg/base/prereq-0.9}}}
• Remove {{$SAGE_ROOT/spkg/base/prereq-0.8.tar}}} (that file is not under revision control).

If that was done, there would be an early test for 'patch'.

In that case, patch-2.5.9.p1.spkg would just need to exit with an exit code of 0 on Cygwin.

Does that sound sensible? I don't think its as good as building patch, but might be easier to implement.

As John says, the documentation would need to document this, but that can be added on another ticket.

As long as the updated .spkg and the changes to $SAGE_ROOT/spkg/base/prereq-0.9-install were merged together, I think that is a reasonable compromise.

Whatever changes are made, please attach a patch to the ticket so we can see what is being done.

Dave

[kcrisman]

comment:15

This is only a problem on Windows 7, as far as we can tell.

[kcrisman]

For review purposes only

[kcrisman]

Author: Dima Pasechnik

[kcrisman]

comment:16

Attachment: 11232-diff.patch.gz

[kcrisman]

comment:17

Sage builds on Cygwin on XP (where it wasn't a problem, however, so we'd need to require patch on all Cygwins) with this spkg, and appears to be building (currently well in, at gsl) on Win 7 with this spkg.

I'm pretty sure that patch is used on packages before that, n'est pas? Given David's comments and the evident nature of the patch to only affect Cygwin, I'd say this is ready for positive review as longa s we can confirm that. Or if my build makes it even further :)

[kcrisman]

comment:18

The build made it all the way. This is Cygwin-only. We will add patch to the list of needs at the wiki and then add all of those things to prereq when Cygwin is actually supported. This seems like a good work flow, that allows continuing unburdened work on the Cygwin port while recognizing that all prerequisites should be found for supported platforms.

[kcrisman]

Changed reviewer from David Kirkby to David Kirkby, Karl-Dieter Crisman

[jdemeyer]

Merged: sage-4.7.2.alpha0

[jdemeyer]

comment:21

New spkg with all files owner-writable: http://boxen.math.washington.edu/home/jdemeyer/spkg/patch-2.5.9.p2.spkg

[jpflori]

comment:22

I think the right solution here would have been to install a patch.exe.manifest file, but I'll implement that in a later ticket.

[dimpase]

comment:23

Replying to @jpflori:

I think the right solution here would have been to install a patch.exe.manifest file, but I'll implement that in a later ticket.

I cannot resist quoting my own comment above:

• It is NOT merely copying an extra file somewhere. One has to use a Microsoft installer to cook up this "security exception", or an equivalent convoluted procedure requiring editing the registry and god knows what else (reading MS documentation on this stuff makes your head spin...)

In short, I don't see a point, as one can just use patch from cygwin instead.

[jpflori]

comment:24

On my system just copying a manifest file is enough.

[jpflori]

comment:25

(And that is exactly what Cygwin does for its own patch IIRC.)

[dimpase]

comment:26

Replying to @jpflori:

(And that is exactly what Cygwin does for its own patch IIRC.)

It was not enough when I tried this; it might depend upon the Windows version (enterprise/business/home/leisure/recreation :-)), type of account, position of the planets, servicepack level, etc etc etc.
I don't think we need to reinvent the wheel by not using what Cygwin provides.

It could also be that if you have one patch.exe installed somewhere, and try to install another one, then you are in trouble...

[jpflori]

comment:27

A subtility we encountered we Jeroen was that the manifest file had to be executable.