Opened 8 years ago

Closed 8 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:

Status badges

Description (last modified by vbraun)

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 of make) in the generated Makefile (and in two scripts called from the Makefile which in turn invoke make). As a consequence, also some parts are now properly built in parallel if make was told to use multiple jobs.
  • Minor clean-up of spkg-install and spkg-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

Attachments (3)

ntl-5.5.2.p0-p1.diff (9.9 KB) - added by leif 8 years ago.
Diff between the .p0 and the .p1. For reference / review only.
ntl-5.5.2.p2.patch (9.5 KB) - added by jpflori 8 years ago.
Spkg diff, for review only.
ntl-p2-p3.diff (5.5 KB) - added by vbraun 8 years ago.
For review purposes only

Download all attachments as: .zip

Change History (40)

Changed 8 years ago by leif

Diff between the .p0 and the .p1. For reference / review only.

comment:1 Changed 8 years ago by leif

  • Cc jpflori added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to 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).

Last edited 8 years ago by jdemeyer (previous) (diff)

comment:3 Changed 8 years ago by jpflori

Same feeling as Jeroen. If you also feel like contacting Shoup to integrate all of our patches, please go ahead :)

comment:4 Changed 8 years ago by leif

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 8 years ago by jpflori

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: Changed 8 years ago by jpflori

  • Dependencies set to #2114
  • Work issues set to 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 in reply to: ↑ 6 Changed 8 years ago by leif

Replying to jpflori:

I'd say the easiest way to go now is to rebase this ticket on the trivial change of #2114.

Au contraire I'd say (if that's correctly spelled).

comment:8 follow-up: Changed 8 years ago by 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)?

Honestly, if you feel all the patches should be restructured (including their documentation), that can be done on another ticket.

comment:9 in reply to: ↑ 8 Changed 8 years ago by jdemeyer

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 8 years ago by jpflori

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:11 Changed 8 years ago by jpflori

I've put an spkg doing all the above and based on #2114 at:

Changed 8 years ago by jpflori

Spkg diff, for review only.

comment:12 Changed 8 years ago by jpflori

  • Authors changed from Leif Leonhardy to Leif Leonhardy, Jean-Pierre Flori
  • Description modified (diff)
  • Keywords ntl added
  • Status changed from needs_work to needs_review
  • Work issues rename and refactor patches, rebase on top of #2114 deleted

comment:13 Changed 8 years ago by leif

  • Reviewers set to Leif Leonhardy
  • Status changed from needs_review to 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 8 years ago by leif

"Patches" is usually a subsection of "Special Update/Build Instructions", i.e., === Patches ===.

comment:15 Changed 8 years ago by leif

And if you like, do

  • patches/make.patch

    a b  
    3131 
    3232 setup4:
    3333-       sh Wizard $(WIZARD)
    34 +       +sh Wizard $(WIZARD) # shell script invoking a Perl script invoking \$MAKE
     34+       +sh Wizard $(WIZARD) # shell script invoking a Perl script invoking $$MAKE
    3535 
    3636 
    3737 ntl.a: $(OBJ)
     
    4444        ./QuickTest
    4545        sh RemoveProg QuickTest
    4646-       sh TestScript
    47 +       +sh TestScript # shell script invoking \$MAKE
     47+       +sh TestScript # shell script invoking $$MAKE
    4848 
    4949 #################################################################
    5050 #

which fixes a minor flaw I only noticed after I had already uploaded my original spkg...

comment:16 follow-up: Changed 8 years ago by jdemeyer

Why did you remove the patches involving LIBTOOL_LINK_FLAGS? This doesn't seem to be documented.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by leif

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 8 years ago by leif

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 in reply to: ↑ 17 ; follow-ups: Changed 8 years ago by 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?

comment:20 Changed 8 years ago by jdemeyer

Making some changes to the spkg, hang on...

comment:21 in reply to: ↑ 19 Changed 8 years ago by leif

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 in reply to: ↑ 19 Changed 8 years ago by leif

Replying to jdemeyer:

Replying to leif:

They're (now) in patches/libtool_flag.patch.

Did you forget to hg add that file?

And yes, fixing .hgignore, indeed one gets:

? patches/libtool_flag.patch

comment:23 Changed 8 years ago by jdemeyer

Better wait for a review of #2114 first, otherwise we can be rebasing forever.

comment:24 Changed 8 years ago by vbraun

  • Milestone changed from sage-5.11 to sage-duplicate/invalid/wontfix
  • Status changed from needs_work to needs_review

Superseded by #14876

comment:25 Changed 8 years ago by vbraun

  • Authors changed from Leif Leonhardy, Jean-Pierre Flori to Leif Leonhardy, Jean-Pierre Flori, Volker Braun
  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-5.11
  • Summary changed from Fix hardcoded 'make' in NTL's build scripts to Fix hardcoded 'make' in NTL's build scripts and track all files

comment:26 Changed 8 years ago by vbraun

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

Changed 8 years ago by vbraun

For review purposes only

comment:27 Changed 8 years ago by jpflori

  • Milestone changed from sage-5.11 to 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 8 years ago by jpflori

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-5.11
  • Reviewers changed from Leif Leonhardy to Leif Leonhardy, Jean-Pierre Flori
  • Status changed from needs_review to needs_work
  • Work issues set to 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: Changed 8 years ago by vbraun

  • Status changed from needs_work to 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 in reply to: ↑ 29 Changed 8 years ago by jpflori

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:31 Changed 8 years ago by jpflori

Maybe we could remove autom4te.cache from the libtool dir.

comment:32 Changed 8 years ago by jpflori

Otherwise everything is fine.

comment:33 Changed 8 years ago by vbraun

I've added a rm -rf autom4te.cache to spkg-src and re-ran it.

comment:34 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:35 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:36 Changed 8 years ago by jdemeyer

  • Work issues spkg-src deleted

comment:37 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.