#11232 closed defect (fixed)
we should not build patch on Cygwin on Windows 7
Reported by: | dimpase | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | porting: Cygwin | Keywords: | |
Cc: | drkirkby | Merged in: | sage-4.7.2.alpha0 |
Authors: | Dima Pasechnik | Reviewers: | David Kirkby, Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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)
Attachments (1)
Change History (28)
comment:1 Changed 10 years ago by
- Cc drkirkby added
- Description modified (diff)
comment:2 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 10 years ago by
- Reviewers set to David Kirkby
- Status changed from needs_review to needs_work
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 10 years ago by
- Status changed from needs_work to needs_review
Replying to 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).
comment:5 in reply to: ↑ 4 Changed 10 years ago by
Replying to dimpase:
Replying to 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.
comment:6 follow-up: ↓ 9 Changed 10 years ago by
- Status changed from needs_review to needs_info
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
comment:7 follow-up: ↓ 8 Changed 10 years ago by
- Status changed from needs_info to needs_work
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
comment:8 in reply to: ↑ 7 Changed 10 years ago by
Replying to 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
comment:9 in reply to: ↑ 6 ; follow-up: ↓ 10 Changed 10 years ago by
- Status changed from needs_work to needs_review
Replying to 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?
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 10 years ago by
Replying to dimpase:
Replying to 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:6you 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
comment:11 in reply to: ↑ 10 ; follow-up: ↓ 12 Changed 10 years ago by
Replying to 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.
comment:12 in reply to: ↑ 11 Changed 10 years ago by
Replying to dimpase:
Replying to 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.
comment:13 Changed 10 years ago by
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).
comment:14 Changed 10 years ago by
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 10th 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
tospkg/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
comment:15 Changed 10 years ago by
- Summary changed from we should not build patch on Cygwin to we should not build patch on Cygwin on Windows 7
This is only a problem on Windows 7, as far as we can tell.
comment:16 Changed 10 years ago by
comment:17 Changed 10 years ago by
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 :)
comment:18 Changed 10 years ago by
- Reviewers changed from David Kirkby to David Kirkby, Karl-Dieter Crisman
- Status changed from needs_review to positive_review
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.
comment:19 Changed 10 years ago by
- Milestone changed from sage-4.7.1 to sage-4.7.2
comment:20 Changed 10 years ago by
- Merged in set to sage-4.7.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:21 Changed 9 years ago by
- Description modified (diff)
New spkg with all files owner-writable: http://boxen.math.washington.edu/home/jdemeyer/spkg/patch-2.5.9.p2.spkg
comment:22 follow-up: ↓ 23 Changed 8 years ago by
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.
comment:23 in reply to: ↑ 22 Changed 8 years ago by
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.
comment:24 Changed 8 years ago by
On my system just copying a manifest file is enough.
comment:25 follow-up: ↓ 26 Changed 8 years ago by
(And that is exactly what Cygwin does for its own patch IIRC.)
comment:26 in reply to: ↑ 25 Changed 8 years ago by
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...
comment:27 Changed 8 years ago by
A subtility we encountered we Jeroen was that the manifest file had to be executable.
The test is not working. I changed it a little, so it worked on Solaris instead of Cygwin. My changes are:
then I created a script called 'patch', which just echos "foo"
Now when I run spkg-install, it does not detect I don't have a usable 'patch; command:
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