Opened 11 years ago

Closed 11 years ago

#9761 closed defect (fixed)

Adjust spkg/standard/deps to build Python before zn_poly, and make two other dependencies explicit

Reported by: mpatel Owned by: GeorgSWeber
Priority: blocker Milestone: sage-4.5.3
Component: build Keywords:
Cc: drkirkby, leif, pjeremy Merged in: sage-4.5.3.alpha2
Authors: David Kirkby Reviewers: Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by leif)

Zn_poly's spkg-install is not a Python script (cf. #9507), but its build system calls a Python script zn_poly-*/src/makemakefile.py. Updating spkg/standard/deps to ensure that Python is built first should prevent build errors in which spkg/logs/zn_poly-*.log ends with

gcc version 4.2.4 (Ubuntu 4.2.4-1ubuntu4)
****************************************************
python: error while loading shared libraries: libpython2.6.so.1.0: cannot open shared object file: No such file or directory
make[2]: Entering directory `/mnt/usb1/scratch/mpatel/tmp/sage-4.5.3.alpha1-ldd/spkg/build/zn_poly-0.9.p5/src'
make[2]: warning: jobserver unavailable: using -j1.  Add `+' to parent make rule.
make[2]: Nothing to be done for `tune'.
make[2]: Leaving directory `/mnt/usb1/scratch/mpatel/tmp/sage-4.5.3.alpha1-ldd/spkg/build/zn_poly-0.9.p5/src'
./spkg-install: line 59: tune/tune: No such file or directory
Error running tune program.

real    0m0.014s
user    0m0.010s
sys     0m0.000s
sage: An error occurred while installing zn_poly-0.9.p5
[...]

Related: Comment 14ff at #9368.


The new deps file also adds two dependencies that were implicit before, and therefore also solves #9464 and #9637.

(The former adds Fortran to R's, the latter adds $BASE to SageTeX's dependencies.)

Attachments (2)

deps (18.2 KB) - added by drkirkby 11 years ago.
New spkg/standard/deps, which resolves this ticket and two minor issues
deps.diff (1.5 KB) - added by drkirkby 11 years ago.
Differences between the new 'deps' file and that in sage-4.5.3.alpha1

Download all attachments as: .zip

Change History (26)

comment:1 follow-up: Changed 11 years ago by mpatel

  • Cc drkirkby leif added

Actually, the error message above appears to stem from my having a SAGE_LOCAL/bin in the PATH but not a corresponding SAGE_LOCAL/lib in the LD_LIBRARY_PATH. I've fixed this by keeping SAGE_LOCAL/* out of both path variables.

Anyway, if an existing [system-wide] Python installation is not a Sage build requirement, I think we should still update deps. Or am I mistaken?

comment:2 in reply to: ↑ 1 Changed 11 years ago by drkirkby

Replying to mpatel:

Actually, the error message above appears to stem from my having a SAGE_LOCAL/bin in the PATH but not a corresponding SAGE_LOCAL/lib in the LD_LIBRARY_PATH. I've fixed this by keeping SAGE_LOCAL/* out of both path variables.

Anyway, if an existing [system-wide] Python installation is not a Sage build requirement, I think we should still update deps. Or am I mistaken?

I agree with you 100%. If zn_poly requires Python then we must ensure Python is built first.

I think this should be a blocker myself, but you are the release manager, so downgrade it if you wish. There are a number of reports of a system Python causing build problems, so in some cases actually disabling Python is necessary (see #9209).

There are two other very trivial fixes to deps which are more cosmetic than anything else, but worth adding, as they aid clarity. These are #9637 and #9464 and we might as well fix all three at once.

Also, SPKG.txt for zn_poly should be updated to reflect the fact there is a dependency on Python. That's a minor issue, but one worth addressing. That is now #9762.

I could make all these changes by around 0100 GMT on Thursday 19th August if you wanted, or could review them sooner if someone else did them. But I've got a lot to do this afternoon, and don't have time to fix them right now. I'm not at home, so don't have my workstation by me, which makes testing things a bit more difficult.

Dave

comment:3 Changed 11 years ago by drkirkby

  • Priority changed from major to blocker

comment:4 follow-up: Changed 11 years ago by leif

Is this documented in zn_poly's SPKG.txt?

If zn_poly('s build system) depends on Python, spkg/standard/deps should of course reflect this; we could in addition ensure in its spkg-install that the current Sage's Python is already available; but see below (w.r.t. Sage's).

I though don't like making packages depend on Python just because some developers appear to be unable or unwilling to write shell scripts instead; the Fortran spkg here is IMHO an especially odd example, since on most systems it's a dummy package, but other packages have to wait until Sage's Python is ready to actually do nothing. (I would completely drop it from the standard distribution; shipping binary executables - in a source tarball! - isn't nice either.)

Btw, William's Fortran problem on bsd.math was (also) due to a broken Python installation/misconfigured environment.

The long-term solution to such should be to generate the Makefile (including deps) in/after Sage's configure. Then we could omit some dependencies (e.g. because a system-wide Python is usable, too), drop [the build of] spkgs like Cephes completely on all systems but Cygwin, and remove ugly things like $PIPESTATUS. So we should take care not to make changes to lots of spkgs we would later have to revert/change, e.g. checking for $SAGE_LOCAL/bin/python; we could use properly set environment variables instead.

Also, spkg/standard/deps (or a file it is generated from, which I'd prefer) should be under revision control.

comment:5 Changed 11 years ago by leif

Concurrent posting... :)

Yes Dave, perhaps create a meta ticket for missing dependencies, (currently) referencing all three changes, and providing a cumulative patch to deps, such that only one ticket has to be merged (the others afterwards closed as "duplicates").

comment:6 in reply to: ↑ 4 ; follow-up: Changed 11 years ago by drkirkby

Replying to leif:

Is this documented in zn_poly's SPKG.txt?

No, but I've created a ticket for that - #9762. That's a low-priority issue.

I though don't like making packages depend on Python just because some developers appear to be unable or unwilling to write shell scripts instead;

I agree. But this is not a spkg-install that depends on Python - it is the upstream source code in a file well hidden. So short of patching the upstream source (which would unwise IMHO), we can't avoid it.

In the case of ATLAS for example, there's a trivial Python spkg-install which calls a huge bash script. I'd like to see that trivial bit of Python converted to a shell script, then we could remove the Python dependency. But the zn_poly issue is different.

Btw, William's Fortran problem on bsd.math was (also) due to a broken Python installation/misconfigured environment.

William seems quite happy to make Python a perquisite for building Sage - something I personally find rather unwise, but he leads the project. I hope enough of us can convince him this is unwise!

Also, spkg/standard/deps (or a file it is generated from, which I'd prefer) should be under revision control.

Agreed. There's a ticket for that I believe.

Anyway, I think the Python dependency should be added to zn_poly. But given there are two other very safe /trivial changes to deps, it would be sensible to do them all at once. We can do it on this ticket I feel. I just don't have time for another 12 hours or so, but can review it if someone else makes the changes.

Dave

comment:7 Changed 11 years ago by leif

I'd suggest adding a target SOME_PYTHON (which currently depends on Sage's Python), and make zn_poly and Fortran depend on that.

This should clarify that (some) Python is needed during the spkg's build process, but not necessarily Sage's one, as opposed to spkgs that really install something into Sage's Python installation, or have to reference it (hard-coded) for later use.

comment:8 follow-ups: Changed 11 years ago by jhpalmieri

I'm not sure that this should be a blocker, since the problem could be viewed as a misconfigured PATH. Furthermore, as Dave says, William is in favor of making a system-wide Python perhaps a prerequisite for building Sage. I think this question should be settled at some point in sage-devel. Then if people decide that Python should be a prerequisite, it should be tested for in prereqs and it should be documented somewhere.

For now, though, I don't think it hurts making zn_poly depend on python.

I though don't like making packages depend on Python just because some developers appear to be unable or unwilling to write shell scripts instead.

Why not? I don't like arbitrarily deciding that people shouldn't write Python scripts. For me, writing, reading, and debugging Python scripts is much easier than doing the same for shell scripts, and I'll probably do a better job working in Python. Why put up barriers for people to contribute, especially when so many contributors are mathematicians who don't particularly want to learn to write shell scripts and who can't remember, for example, whether to use "-a" or "&&"?

comment:9 Changed 11 years ago by leif

  • Cc pjeremy added

src/configure is nice:

#!/bin/sh

#
# This script just calls makemakefile.py.
# See that file for the real configure script.
#

if test $# -ne 0
then
   python makemakefile.py "$@" > makefile
else
   python makemakefile.py > makefile
fi

Looking at that Python script:

~/Sage/spkgs/zn_poly-0.9.p5$ wc -l src/makemakefile.py 
274 src/makemakefile.py
~/Sage/spkgs/zn_poly-0.9.p5$ grep -c print src/makemakefile.py 
119
~/Sage/spkgs/zn_poly-0.9.p5$ grep -c "^#" src/makemakefile.py 
31
~/Sage/spkgs/zn_poly-0.9.p5$ grep -c "^$" src/makemakefile.py 
47

Much of the rest just defines lists of strings (filenames), and afterwards prepends some directory/path to each of the list elements. Then some strings are concatenated, partly with case distinctions. (Ok, option parsing is another job done, but most of it should be in a Makefile.in.)

I wonder if we could simply replace this by (making use of) a pregenerated file for the purpose of Sage, using some shell variables. IMHO requiring Python here is like requiring autotools to be present.

Btw, from spkg-install it is pretty clear that this package requires Python:

$ grep makemake spkg-install 
   cp patches/makemakefile.py src/makemakefile.py
   sed 's/-soname/-h/g' src/makemakefile.py > /tmp/makemakefile.py.$$
   mv /tmp/makemakefile.py.$$ src/makemakefile.py 

Just my needless 2 cents...

comment:10 in reply to: ↑ 8 Changed 11 years ago by leif

Replying to jhpalmieri:

I'm not sure that this should be a blocker, since the problem could be viewed as a misconfigured PATH.

Well, if no system Python was available, the build would (or could) break, too.

Furthermore, as Dave says, William is in favor of making a system-wide Python perhaps a prerequisite for building Sage.

I wouldn't mind, or perhaps even appreciate that. Like gcc requiring a C compiler to build... (I though wonder if we could then drop the Python package from Sage, at least if a suitable version is already present.)

I though don't like making packages depend on Python just because some developers appear to be unable or unwilling to write shell scripts instead.

Why not?

Because - currently - Python is not a prerequisite for building Sage.

I don't like arbitrarily deciding that people shouldn't write Python scripts. For me, writing, reading, and debugging Python scripts is much easier than doing the same for shell scripts, and I'll probably do a better job working in Python.

I didn't mean that; in this case, the build process, IMHO one should use tools that are designed for the specific purpose.

Why put up barriers for people to contribute, especially when so many contributors are mathematicians who don't particularly want to learn to write shell scripts and who can't remember, for example, whether to use "-a" or "&&"?

There are some Sage developers that aren't mathematicians; hopefully these could give help with the non-mathematical parts. (Collaboration between different disciplines seems to be a never-ending problem...)

It's easier to read Makefiles (and for me, e.g. configure scripts, too) than arbitrary Python/Ruby/Perl?/BASIC?/... scripts, especially when people feel they have to reinvent their own wheel, i.e., not even using libraries or packages that already achieve the same in a more standard way (though I'm personally not very happy with e.g. SCons).

Once Sage has "boot-strapped", everybody is free to do arbitrary things in the languages Sage supports...

comment:11 in reply to: ↑ 6 Changed 11 years ago by leif

Replying to drkirkby:

In the case of ATLAS for example, there's a trivial Python spkg-install which calls a huge bash script. I'd like to see that trivial bit of Python converted to a shell script, then we could remove the Python dependency.

Ouch...

I believe I've never before looked at Sage's ATLAS build scripts. (I did not even know the repetition of build attempts originated from Sage itself.) There are three Python and some (sub-)shell scripts, and there's even a Perl script. 8|

This should really be cleaned up, IMHO by completely rewriting spkg-install from scratch, as a plain shell script.

comment:12 in reply to: ↑ 8 Changed 11 years ago by drkirkby

Replying to jhpalmieri:

I'm not sure that this should be a blocker, since the problem could be viewed as a misconfigured PATH.

I'm writing this quick - due in a meeting soon.

I believe it should be a blocker as Python is not listed as a prequisite for building Sage. So zn_poly could fail if Python is not installed. That, in my opinion is good enough reason.

Other reasons are

  • It's a trivial fix.
  • Mis-configured Pythons do exist - William's on bsd.math was one example.
  • I also happen to be aware of cases where having a Python in /usr/local can cause Sage to mis-behave. I think I have posted a link to the ticket on here, where this is documented. Both myself and a Linux user has seen this.

But the main reason is simply that Python is not a perquisite. All other issues are secondary in comparison to that.

Dave

comment:13 follow-up: Changed 11 years ago by mpatel

Let's keep "adding Python to zn_poly's build dependencies" as a 4.5.3 blocker. Rolling #9637 and #9464 into a deps patch here is OK with me.

Just to check on #9464: Are we generally going to be explicit about all dependencies or just potential sources of confusion?

comment:14 Changed 11 years ago by drkirkby

I'm attaching a patch (unified diff, not Mercurial), which addresses three issues.

  • Makes ZNPOLY dependent on PYTHON - the point of this ticket.
  • Makes SAGETEX depend on BASE. Not strictly necessary, but its the only package which does not list BASE specifically. This is #9637
  • Make R depend on FORTRAN. This is not strictly necessary, as it is implied by a very convoluted chain rule, but this makes it more obvious. This is #9464

If this gets merged, both #9637 and #9464 can be closed.

Since spkg/standard/deps is not under revision control, this will have to be merged manually.

To answer John's point, I don't have a strong view on that either way, but certainly if there is any potential for confusion, then I think we should list them explicitly.

Dave

Changed 11 years ago by drkirkby

New spkg/standard/deps, which resolves this ticket and two minor issues

Changed 11 years ago by drkirkby

Differences between the new 'deps' file and that in sage-4.5.3.alpha1

comment:15 Changed 11 years ago by drkirkby

  • Status changed from new to needs_review

comment:16 Changed 11 years ago by drkirkby

  • Authors set to David Kirkby

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

Replying to mpatel:

Just to check on #9464: Are we generally going to be explicit about all dependencies or just potential sources of confusion?

I'd say making direct dependencies explicit is not a bad idea. In addition, as said above, I'd prefer differentiating between tools/packages etc. needed during build and those that are necessary for later operation, at least e.g. for Python.

Of course this could be subdivided further, but does not yet make many sense.

(All standard packages currently depend on $BASE rather than a set of the actually required things; this is also not very informative, but I wouldn't change that at the moment.)

comment:18 Changed 11 years ago by drkirkby

Sorry,

I see it was Mitesh's point about whether we should generally be explicit about all dependencies or just potential sources of confusion - not John's.

I'm unsure what to say about that one. I supposed putting them all in would avoid confusion. It makes the file bigger, and the mere fact of making the changes could introduce a bug.

A difficult one - I'll sit on the fence!

Dave

comment:19 follow-up: Changed 11 years ago by leif

  • Reviewers set to Leif Leonhardy
  • Status changed from needs_review to positive_review

I would have liked a SOME_PYTHON target, but the comment on zn_poly's rule should be enough for the moment.

I've verified that the attached deps file is actually 4.5.3.alpha1's with deps.diff's changes applied, so positive review.

comment:20 Changed 11 years ago by leif

  • Description modified (diff)
  • Summary changed from Adjust spkg/standard/deps to build Python before zn_poly to Adjust spkg/standard/deps to build Python before zn_poly, and make two other dependencies explicit

comment:21 in reply to: ↑ 19 ; follow-up: Changed 11 years ago by drkirkby

Replying to leif:

I would have liked a SOME_PYTHON target, but the comment on zn_poly's rule should be enough for the moment.

I'm not sure a SOME_PYTHON target is a good idea myself. But anyway, that is far outside the scope of this ticket. Such a change needs discussion on sage-devel - not a trac ticket.

I've verified that the attached deps file is actually 4.5.3.alpha1's with deps.diff's changes applied, so positive review.

Thank you for the positive review Leif.

Is there any chance you could complete your review of #9603? In return, I'll fix #9762, which you clearly thought was more important than me! Then we both get some changes we want.

Dave

comment:22 in reply to: ↑ 21 Changed 11 years ago by leif

Replying to drkirkby:

Replying to leif:

I would have liked a SOME_PYTHON target, but the comment on zn_poly's rule should be enough for the moment.

I'm not sure a SOME_PYTHON target is a good idea myself. But anyway, that is far outside the scope of this ticket. Such a change needs discussion on sage-devel - not a trac ticket.

For now, I just meant:

  • deps

    old new  
    129129                         $(INST)/$(DIR)
    130130        $(INSTALL) "$(SAGE_SPKG) $(SAGE_SCRIPTS) 2>&1" "tee -a $(SAGE_LOGS)/$(SAGE_SCRIPTS).log"
    131131
     132# A (perhaps system-wide) Python that's only used for scripting
     133# in the build process of a Sage package:
     134some_python: $(INST)/$(PYTHON)
     135        # We currently only use Sage's Python, even if another
     136        # (functional) Python is already present.
     137
    132138########################################
    133139# Building normal packages
    134140########################################
     
    374380
    375381# zn_poly really does depend on Python, despite this is far from obvious.
    376382# The 'configure' script in zn_poly calls Python to make a 'makefile'
    377 $(INST)/$(ZNPOLY): $(BASE) $(INST)/$(MPIR) $(INST)/$(PYTHON)
     383$(INST)/$(ZNPOLY): $(BASE) $(INST)/$(MPIR) some_python
    378384        $(INSTALL) "$(SAGE_SPKG) $(ZNPOLY) 2>&1" "tee -a $(SAGE_LOGS)/$(ZNPOLY).log"
    379385
    380386# setuptools forgets to update easy-install.pth during parallel
     
    468474        $(INSTALL) "$(SAGE_SPKG) $(SAGE) 2>&1" "tee -a $(SAGE_LOGS)/$(SAGE).log"
    469475
    470476# Do not remove PYTHON below -- see trac 9368
    471 $(INST)/$(FORTRAN): $(BASE)  $(INST)/$(PYTHON)
     477$(INST)/$(FORTRAN): $(BASE) some_python
    472478        $(INSTALL) "$(SAGE_SPKG) $(FORTRAN) 2>&1" "tee -a $(SAGE_LOGS)/$(FORTRAN).log"
    473479
    474480$(INST)/$(F2C): $(BASE) $(INST)/$(FORTRAN)

This makes the reason for the dependency more clear - even without further comments, and the rule for some_python can simply be changed in the future to actually make use of another Python - without the need for changing other rules.

And it's formal, not prose.


I'll fix #9762, which you clearly thought was more important than me!

QA says it should definitely be a blocker. ;-)

comment:23 in reply to: ↑ 17 Changed 11 years ago by drkirkby

Replying to leif:

Replying to mpatel:

Just to check on #9464: Are we generally going to be explicit about all dependencies or just potential sources of confusion?

I'd say making direct dependencies explicit is not a bad idea.

I've just thought of a reason why it is a good idea to be explicit about all dependencies. Whilst before I was sitting on the fence over this, I now believe there are advantages in listing every dependency directly, for the reasons stated below.

I wanted to start a build of Sage, without building ATLAS, so I copied the ATLAS libraries & headers from a previous Sage build, into a directory and touched spkg/installed/atlas-3.8.3.p14. Now, when I start to build Sage, neither Fortran, lapack or python are built.

But something that required Fortran, lapack or Python, but only listed ATLAS as a dependency, could then fail to build.

Like it or not, people do sometimes copy parts of one build to another, in order to save time. (Especially with ATLAS). That is more risky if spkg/standard/deps does not explicitly list each and every dependency.

Dave

comment:24 Changed 11 years ago by mpatel

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