Opened 10 years ago
Closed 10 years ago
#14692 closed defect (fixed)
Fix hardcoded 'make' in NTL's build scripts and track all files
Reported by: | leif | Owned by: | leif |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.12 |
Component: | packages: standard | Keywords: | spkg ntl |
Cc: | jpflori | Merged in: | sage-5.12.beta0 |
Authors: | Leif Leonhardy, Jean-Pierre Flori, Volker Braun | Reviewers: | Leif Leonhardy, Jean-Pierre Flori |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #2114 | Stopgaps: |
Description (last modified by )
New spkg: http://boxen.math.washington.edu/home/vbraun/spkg/ntl-5.5.2.p3.spkg
ntl-5.5.2.p2 (Leif Leonhardy, Jean-Pierre Flori, 6 June 2013)
- #14692: Patch upstream to use
$(MAKE)
(instead ofmake
) in the generated Makefile (and in two scripts called from the Makefile which in turn invokemake
). As a consequence, also some parts are now properly built in parallel ifmake
was told to use multiple jobs. - Minor clean-up of
spkg-install
andspkg-check
. - Refactor and rename patches.
ntl-5.5.2.p3 (Volker Braun, July 10 2013)
- #14692: Add spkg-src and track all files.
For review purposes
- p0->p1: ntl-5.5.2.p0-p1.diff
- p1->p2: ntl-5.5.2.p2.patch
- p2->p3: ntl-p2-p3.diff
Attachments (3)
Change History (40)
Changed 10 years ago by
Attachment: | ntl-5.5.2.p0-p1.diff added |
---|
comment:1 Changed 10 years ago by
Cc: | jpflori added |
---|---|
Description: | modified (diff) |
Status: | new → needs_review |
comment:2 Changed 10 years ago by
Status: | needs_review → needs_work |
---|
I think it would be much better to have just one patch fixing this issue, instead of 3 (one per file). This makes it easier to document what the patch does (SPKG.txt
should mention this patch).
Also, having just one patch makes it easier to report upstream (which should be done).
comment:3 Changed 10 years ago by
Same feeling as Jeroen. If you also feel like contacting Shoup to integrate all of our patches, please go ahead :)
comment:4 Changed 10 years ago by
That's (to some extent) a matter of taste, although I actually prefer [cumulative] patches with "verbose names", too. (It only sometimes gets messy when different patches patch the same files, and especially when they're applied in some "random" order.)
But there was already patches/mfile.patch
(fixing Cygwin stuff*), so I followed the scheme that was already there.
* Does anyone recall OTOH who added that? XD
comment:5 Changed 10 years ago by
I guess I'm the culprit for the Cygwin patch... As all the previous patches only have one purpose and touch only one file, it may be a good time to rename them, DoConfig? and mfile are not really talkative.
Of course patching the same file multiple times with your new set of patches might be problematic.
comment:6 follow-up: 7 Changed 10 years ago by
Dependencies: | → #2114 |
---|---|
Work issues: | → rename and refactor patches, rebase on top of #2114 |
I'd say the easiest way to go now is to rebase this ticket on the trivial change of #2114.
comment:7 Changed 10 years ago by
comment:8 follow-up: 9 Changed 10 years ago by
Is "refactor and rename patches" in any way related to the ticket's subject, namely fixing a defect (which the current spkg properly does)?
Honestly, if you feel all the patches should be restructured (including their documentation), that can be done on another ticket.
comment:9 Changed 10 years ago by
Replying to leif:
Is "refactor and rename patches" in any way related to the ticket's subject, namely fixing a defect (which the current spkg properly does)?
Perhaps what is meant is simply my comment about having one patch to fix make
and documenting that patch in SPKG.txt
.
comment:10 Changed 10 years ago by
Yeah, let's just do that.
To properly refactor we should split the libtool part in DoConfig?.patch and merge it with mfile.patch into a libtool_flag.patch later and put the remaining part into a disable_help.patch. Maybe move new.h.patch to new_singular.patch. And properly document everything in SPKG.txt by reflecting what is done by which patch (although the names should be then more talkative). But I can do that in a folow up ticket.
comment:12 Changed 10 years ago by
Authors: | Leif Leonhardy → Leif Leonhardy, Jean-Pierre Flori |
---|---|
Description: | modified (diff) |
Keywords: | ntl added |
Status: | needs_work → needs_review |
Work issues: | rename and refactor patches, rebase on top of #2114 |
comment:13 Changed 10 years ago by
Reviewers: | → Leif Leonhardy |
---|---|
Status: | needs_review → needs_work |
patches/make.patch
lacks the patch to src/src/TestScript
.
(I'd also rename the patch to use_MAKE.patch
or smth like that.)
comment:14 Changed 10 years ago by
"Patches" is usually a subsection of "Special Update/Build Instructions", i.e., === Patches ===
.
comment:15 Changed 10 years ago by
And if you like, do
-
patches/make.patch
a b 31 31 32 32 setup4: 33 33 - sh Wizard $(WIZARD) 34 + +sh Wizard $(WIZARD) # shell script invoking a Perl script invoking \$MAKE34 + +sh Wizard $(WIZARD) # shell script invoking a Perl script invoking $$MAKE 35 35 36 36 37 37 ntl.a: $(OBJ) … … 44 44 ./QuickTest 45 45 sh RemoveProg QuickTest 46 46 - sh TestScript 47 + +sh TestScript # shell script invoking \$MAKE47 + +sh TestScript # shell script invoking $$MAKE 48 48 49 49 ################################################################# 50 50 #
which fixes a minor flaw I only noticed after I had already uploaded my original spkg...
comment:16 follow-up: 17 Changed 10 years ago by
Why did you remove the patches involving LIBTOOL_LINK_FLAGS
? This doesn't seem to be documented.
comment:17 follow-up: 19 Changed 10 years ago by
Replying to jdemeyer:
Why did you remove the patches involving
LIBTOOL_LINK_FLAGS
?
? They're (now) in patches/libtool_flag.patch
.
This doesn't seem to be documented.
comment:18 Changed 10 years ago by
Still, we have
* libtool_flag.patch: modify mfile to enable the build of a shared library on Cygwin
but
$ grep '^+++' patches/libtool_flag.patch +++ src/src/DoConfig 2012-08-07 12:17:12.828358360 +0200 +++ src/src/mfile 2013-06-05 20:43:25.845240069 +0200
I.e., add DoConfig
to the patch's documentation.
comment:19 follow-ups: 21 22 Changed 10 years ago by
Replying to leif:
They're (now) in
patches/libtool_flag.patch
.
I don't see that file in ntl-5.5.2.p2.patch. Did you forget to hg add
that file?
comment:21 Changed 10 years ago by
Replying to jdemeyer:
Replying to leif:
They're (now) in
patches/libtool_flag.patch
.I don't see that file in ntl-5.5.2.p2.patch. Did you forget to
hg add
that file?
.hgignore
is the problem. (It contains libtool
rather than libtool/
.)
comment:22 Changed 10 years ago by
comment:23 Changed 10 years ago by
Better wait for a review of #2114 first, otherwise we can be rebasing forever.
comment:24 Changed 10 years ago by
Milestone: | sage-5.11 → sage-duplicate/invalid/wontfix |
---|---|
Status: | needs_work → needs_review |
Superseded by #14876
comment:25 Changed 10 years ago by
Authors: | Leif Leonhardy, Jean-Pierre Flori → Leif Leonhardy, Jean-Pierre Flori, Volker Braun |
---|---|
Description: | modified (diff) |
Milestone: | sage-duplicate/invalid/wontfix → sage-5.11 |
Summary: | Fix hardcoded 'make' in NTL's build scripts → Fix hardcoded 'make' in NTL's build scripts and track all files |
comment:26 Changed 10 years ago by
Description: | modified (diff) |
---|
Since the new upstream version is incompatible with our Singular, lets make a new spkg here with the fixes needed for the git transition. The version bump can wait until Singular is ready.
comment:27 Changed 10 years ago by
Milestone: | sage-5.11 → sage-duplicate/invalid/wontfix |
---|
Is the "Make everything writable" part actually needed? I guess so, or you wouldn't have added it, but I was not aware of that.
comment:28 Changed 10 years ago by
Milestone: | sage-duplicate/invalid/wontfix → sage-5.11 |
---|---|
Reviewers: | Leif Leonhardy → Leif Leonhardy, Jean-Pierre Flori |
Status: | needs_review → needs_work |
Work issues: | → spkg-src |
It would be nice to have a way to use a local copy of ntl tarball in spkg-src (just as you did for Singular in #14737).
comment:29 follow-up: 30 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
I haven't checked if the "make writeable" part is really needed. But since it also depends on umask and since wrong permissions in tarballs are not that uncommon, I think its better to include it.
The ntl tarball is tiny and on a fast server, so I didn't need an option to use a local copy for writing/testing the script. If you need it, feel free to open a ticket with that feature request.
comment:30 Changed 10 years ago by
Replying to vbraun:
I haven't checked if the "make writeable" part is really needed. But since it also depends on umask and since wrong permissions in tarballs are not that uncommon, I think its better to include it.
The ntl tarball is tiny and on a fast server, so I didn't need an option to use a local copy for writing/testing the script. If you need it, feel free to open a ticket with that feature request.
Except when the site is down :)
comment:34 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
comment:35 Changed 10 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:36 Changed 10 years ago by
Work issues: | spkg-src |
---|
comment:37 Changed 10 years ago by
Merged in: | → sage-5.12.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Diff between the
.p0
and the.p1
. For reference / review only.