Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#9799 closed defect (fixed)

Running "make" in SAGE_ROOT returns the wrong exit code, leading to all kinds of confusion

Reported by: was Owned by: GeorgSWeber
Priority: critical Milestone: sage-4.6.1
Component: build Keywords: makefile
Cc: leif, jdemeyer, drkirkby, fbissey Merged in: sage-4.6.1.alpha0
Authors: Leif Leonhardy, John Palmieri Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This line in SAGE_ROOT/make

cd spkg && ./install all 2>&1 | tee -a ../install.log

will *always* return an exit status of 0, even if the build fails miserably. This leads to much confusion down the line, as explained in this sage-devel post: http://groups.google.com/group/sage-devel/browse_thread/thread/3487f96fda36b6f0/09be2d4dc50493f1#09be2d4dc50493f1

The fix is probably to do what is described here:

http://www.unix.com/shell-programming-scripting/92163-command-does-not-return-exit-status-due-tee.html

As part of the patch, makefile should be renamed to Makefile (this is a more standard name).

Follow-up tickets: #10156, #10157

Attachments (7)

makefile (3.2 KB) - added by jhpalmieri 9 years ago.
makefile.diff (2.1 KB) - added by jhpalmieri 9 years ago.
makefile.9799-ll-v2 (4.8 KB) - added by leif 9 years ago.
Revised version of John's Makefile, with many of the other changes I mentioned.
makefile.jhp-ll-v2.diff (6.0 KB) - added by leif 9 years ago.
Diff between John's and my Makefile.
makefile.9799-v3 (4.9 KB) - added by jhpalmieri 9 years ago.
version 3 of the makefile (or Makefile)
makefile.v3.diff (1.1 KB) - added by jhpalmieri 9 years ago.
Diff between v2 and v3
9799_makefile.patch (5.8 KB) - added by jdemeyer 9 years ago.
Complete patch from the makefile in sage-4.6.alpha3

Download all attachments as: .zip

Change History (41)

comment:1 Changed 9 years ago by was

  • Description modified (diff)

comment:2 Changed 9 years ago by mpatel

  • Cc leif added

I think we already have nearly all we need in

SAGE_ROOT/spkg/pipestatus

which we use often in

SAGE_ROOT/spkg/standard/deps

For upgrades from Sage versions without pipestatus: What if we create pipestatus in the makefile instead of in spkg/install?

Leif mentioned using pipestatus or the equivalent in the makefile here.

comment:3 Changed 9 years ago by jhpalmieri

Well, if we could merge #9433, then I think that pipestatus would get installed through the upgrade process.

comment:4 follow-up: Changed 9 years ago by jhpalmieri

  • Status changed from new to needs_review

For upgrades from Sage versions without pipestatus: What if we create pipestatus in the makefile instead of in spkg/install?

I may be wrong about this, but if you run "sage -upgrade", then I don't think "makefile" gets upgraded. So if we just modify makefile, we can assume that we have pipestatus available to us: people upgrading won't get the new makefile, and people building from scratch will have spkg/pipestatus.

So I'm attaching a new makefile and corresponding makefile.diff. I'm far from an expert at writing makefiles, so there are probably cleaner ways to do this.

Changed 9 years ago by jhpalmieri

Changed 9 years ago by jhpalmieri

comment:5 Changed 9 years ago by leif

Not yet tested, but the changes so far look ok to me.

I'd add dependencies on spkg/pipestatus, and perhaps a rule / receipt that makes sure it is (present and) executable. (More of such dependencies could be added as well.)

Perhaps substitute

        cd spkg && ./pipestatus "./install all 2>&1" "tee -a ../install.log"

by

        spkg/pipestatus "cd spkg && ./install all 2>&1" "tee -a install.log"

and, optionally, use a Make variable for spkg/pipestatus.

Some changes we should also make:

  • distclean should depend on clean, not call make (if at all, $(MAKE)).
  • The dependency of all on build is redundant.
  • s/Deleted/Deleting/g or move those messages.
  • makefile is not a phony target.
  • ./sage -b is not consistently performed, and btw. after the documentation has been built / updated. Another target and dependency on that would be better.
  • Targets testoptional (logfile testall.log) and ptestall (logfile ptest.log!) are inconsistent.

Changed 9 years ago by leif

Revised version of John's Makefile, with many of the other changes I mentioned.

Changed 9 years ago by leif

Diff between John's and my Makefile.

comment:6 Changed 9 years ago by leif

I've attached a modified version of John's Makefile, and a corresponding diff.

Not yet very much tested, but should work... ;-)

Ceterum censeo makefile esse renominandum? (i.e. Makefile)

comment:7 Changed 9 years ago by leif

Oh, testall and ptestoptional should also be added to the phony targets.

(I only added ptest, which was missing, and removed makefile.)

comment:8 follow-up: Changed 9 years ago by jhpalmieri

I've tested this a little bit, and it basically seems to work for me. I also have no objections to renaming it Makefile. (If it does get renamed, then #9433 will need to be changed accordingly.) I'm attaching version 3; this just adds Leif's requested phony targets, plus changes some tabs to spaces. With the tabs, I see this at the start of the build:

(sage-4.5.3) [10:58]$ make
# Note that (currently) "tee" will be run in the directory cd'ed to
# in pipestatus' first argument, i.e. "spkg/": 
spkg/pipestatus "cd spkg && ./install all 2>&1" "tee -a ../install.log"

With spaces instead, the lines starting with "#" do not appear.

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

Replying to jhpalmieri:

I've tested this a little bit, and it basically seems to work for me. I also have no objections to renaming it Makefile.

Fine.

(If it does get renamed, then #9433 will need to be changed accordingly.)

Yes, that's already recorded there.

I'm attaching version 3; this just adds Leif's requested phony targets,

Thanks.

plus changes some tabs to spaces. With the tabs, I see this at the start of the build:

(sage-4.5.3) [10:58]$ make
# Note that (currently) "tee" will be run in the directory cd'ed to
# in pipestatus' first argument, i.e. "spkg/": 
spkg/pipestatus "cd spkg && ./install all 2>&1" "tee -a ../install.log"

Well, that's (just) informational and does no harm... ;-)

With spaces instead, the lines starting with "#" do not appear.

I'm currently not that sure that would be very portable; instead, you could also prepend @ (to #, and leave the tabs). The comments were actually shell comments (i.e., part of the receipt) rather than Makefile comments.

comment:10 follow-up: Changed 9 years ago by jhpalmieri

Well, that's (just) informational and does no harm... ;-

I think it could be a little confusing: it looks like it should be informational, but it won't mean much to some users, and so they might be concerned or worried or puzzled. If we skip it, then the first printed line looks like something technical which therefore be safely ignored by uninterested people...

I'm attaching new versions with "@#".

comment:11 Changed 9 years ago by jhpalmieri

(I'm assuming that "interested" people will start browsing through the Makefile, so will see the comment...)

comment:12 in reply to: ↑ 10 Changed 9 years ago by leif

Replying to jhpalmieri:

Well, that's (just) informational and does no harm... ;-

I think it could be a little confusing [...]

Agreed.

Changed 9 years ago by jhpalmieri

version 3 of the makefile (or Makefile)

Changed 9 years ago by jhpalmieri

Diff between v2 and v3

comment:13 Changed 9 years ago by jhpalmieri

I changed one more "#" to "@#": without the "@", make test print this possibly confusing message:

# . local/bin/sage-env && sage-starts && (also) puts sage-maketest into the path

It reads better as a comment, with no evaluation of the variable:

     @# $(TESTPRELIMS) (also) puts sage-maketest into the path

comment:14 in reply to: ↑ 4 Changed 9 years ago by mpatel

Replying to jhpalmieri:

For upgrades from Sage versions without pipestatus: What if we create pipestatus in the makefile instead of in spkg/install?

I may be wrong about this, but if you run "sage -upgrade", then I don't think "makefile" gets upgraded. So if we just modify makefile, we can assume that we have pipestatus available to us: people upgrading won't get the new makefile, and people building from scratch will have spkg/pipestatus.

You're right. Your idea is simpler and better.

I can't test V3 right now, but here or elsewhere, should we rewrite the test target so it works like the other testing targets?

On renaming makefile: Does any platform (Windows?) require the current name?

comment:15 Changed 9 years ago by jdemeyer

  • Cc jdemeyer added

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

What is the status of this ticket? It looks like there are some good ideas here, let's not waste this effort... Is this related to #9896 or is it independent of that?

comment:17 in reply to: ↑ 16 Changed 9 years ago by leif

Replying to jdemeyer:

What is the status of this ticket? It looks like there are some good ideas here, let's not waste this effort... Is this related to #9896 or is it independent of that?

This one is rather unrelated (or even better, afaik independent of #9896).

Feel free to improve the Makefile further, or review what we have so far... ;-) (See some comments above.)

The (or a) "concurrent" ticket to this (and #9896) is "Put more files under revision control", #9433, which I'd like to see merged in 4.6.1.

(Also related: #9811, which is I guess caused by teeing without pipestatus as well.)

comment:18 Changed 9 years ago by leif

  • Cc drkirkby fbissey added

Something to review... ;-)

Changed 9 years ago by jdemeyer

Complete patch from the makefile in sage-4.6.alpha3

comment:19 follow-up: Changed 9 years ago by jdemeyer

  • Authors set to John Palmieri
  • Keywords makefile added
  • Reviewers set to Jeroen Demeyer

I have read the patch and it looks okay to me, but perhaps somebody who is more familiar with the Sage build process should have a second look. I am currently doing a full build from scratch with the new Makefile (with capital). I have a few comments (which you're free to ignore...).

Why not leave

all: build doc 

as it is? I agree that "build" is superfluous, but I think it adds clarity. It is also more robust in case other parts of the makefile are changed.

the same for

doc-html: build # (already) indirectly depends on $(PIPE)

About the comments: I think it more common to put them outside the make rules, so instead of

build:  $(PIPE)
    @# Note that (currently) "tee" will be run in the directory cd'ed to 
    @# in pipestatus' first argument, i.e. "spkg/":  
    $(PIPE) "cd spkg && ./install all 2>&1" "tee -a ../install.log" 

why not write

# Note that (currently) "tee" will be run in the directory cd'ed to
# in pipestatus' first argument, i.e. "spkg/":
build:  $(PIPE)
    $(PIPE) "cd spkg && ./install all 2>&1" "tee -a ../install.log" 

comment:20 Changed 9 years ago by jhpalmieri

  • Authors changed from John Palmieri to Leif Leonhardy, John Palmieri

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

Replying to jdemeyer:

Why not leave

all: build doc 

as it is? I agree that "build" is superfluous, but I think it adds clarity.

A matter of taste, though I think making just doc an explicit prerequisite of all is more clear here, since it is rather uncommon that the documentation depends on compiling all of the code. (For spkg/standard/deps, I prefer at least some redundancy because it's partially obscure what depends on what. Especially omitting true direct dependencies because they're already accomplished through deep indirect dependencies is quite error-prone, since in fact there dependencies are more likely to change than here. Analogous to the new Fedora library dependency scheme.)

It is also more robust in case other parts of the makefile are changed.

See above; I don't think that's the case here, since the dependencies are rather trivial and clear from the context. ;-)

the same for

doc-html: build # (already) indirectly depends on $(PIPE)

I agree on adding $(PIPE), saves some characters (the comment) anyway. ;-)

About the comments: I think it more common to put them outside the make rules, so instead of

build:  $(PIPE)
    @# Note that (currently) "tee" will be run in the directory cd'ed to 
    @# in pipestatus' first argument, i.e. "spkg/":  
    $(PIPE) "cd spkg && ./install all 2>&1" "tee -a ../install.log" 

why not write

# Note that (currently) "tee" will be run in the directory cd'ed to
# in pipestatus' first argument, i.e. "spkg/":
build:  $(PIPE)
    $(PIPE) "cd spkg && ./install all 2>&1" "tee -a ../install.log" 

Well, the comment actually refers solely to the single shell command line, not the rule. Just imagine there were more of them...

comment:22 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Installing with the patched Makefile worked fine, also tried to see what happens if make, make doc or make ptest fails. This does return an error code as it should. So positive_review.

comment:23 follow-up: Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:24 in reply to: ↑ 23 ; follow-ups: Changed 9 years ago by leif

Replying to jdemeyer:

And the reason for this is that files like Makefile and README etc. (should) appear first in directory listings, therefore uppercase.

Btw, I wonder if we should at all let the mixed-up output appear on the screen (i.e., stdout) when doing parallel builds. This isn't that easy to handle, because the "decision" to actually perform a parallel build is currently (IMHO unnecessarily) made in spkg/install. If we want to change that (I'd prefer having just "Building package xy...", "Successfully installed package xy" / "Error installing package xy" on the screen and in the main log, $SAGE_ROOT/install.log), this should of course be addressed on another ticket.

It's currently hard to see which package actually failed to install after getting "Error building Sage."; grepping for "Error" gives a lot of harmless lines.

comment:25 in reply to: ↑ 24 Changed 9 years ago by leif

Replying to leif:

It's currently hard to see which package actually failed to install after getting "Error building Sage."; grepping for "Error" gives a lot of harmless lines.

We could at least (on errors) print a summary at the end, analogous to doctesting ("The following packages failed to build/install: ..."). This would currently be a bit easier to implement (by keeping track of which packages are to be built, and at the end checking if they were successfully built).

comment:26 in reply to: ↑ 24 ; follow-up: Changed 9 years ago by mpatel

Replying to leif:

Btw, I wonder if we should at all let the mixed-up output appear on the screen (i.e., stdout) when doing parallel builds. This isn't that easy to handle, because the "decision" to actually perform a parallel build is currently (IMHO unnecessarily) made in spkg/install. If we want to change that (I'd prefer having just "Building package xy...", "Successfully installed package xy" / "Error installing package xy" on the screen and in the main log, $SAGE_ROOT/install.log), this should of course be addressed on another ticket.

Would using, e.g.,

        @echo '  ACTION  ' $@; command that does the action

in Makefile and deps and replacing "pipestatus/tee" with ">>" in deps help? Is there any way to keep recording a big, multiplexed log in the background, if we still want it?

comment:27 in reply to: ↑ 26 ; follow-up: Changed 9 years ago by leif

Replying to mpatel:

Replying to leif:

Btw, I wonder if we should at all let the mixed-up output appear on the screen (i.e., stdout) when doing parallel builds. This isn't that easy to handle, because the "decision" to actually perform a parallel build is currently (IMHO unnecessarily) made in spkg/install. If we want to change that (I'd prefer having just "Building package xy...", "Successfully installed package xy" / "Error installing package xy" on the screen and in the main log, $SAGE_ROOT/install.log), this should of course be addressed on another ticket.

Would using, e.g.,

        @echo '  ACTION  ' $@; command that does the action

in Makefile and deps and replacing "pipestatus/tee" with ">>" in deps help? Is there any way to keep recording a big, multiplexed log in the background, if we still want it?

I was thinking of conditionally teeing (at least) in the top-level Makefile (just redirecting stdout on parallel builds) and checking the exit status inside the receipts to print appropriate messages to some other file descriptor than stdout in deps.

Verbosity levels for sage-spkg wouldn't be bad either, especially suppressing the verbose tar output is IMHO worth doing, since a lot of spkgs contain hundreds or thousands of files (while the actual build log is often only a few lines).

But I must admit I've already forgotten what exactly I'd planned yesterday... ;-)

comment:28 in reply to: ↑ 27 ; follow-up: Changed 9 years ago by drkirkby

Replying to leif:

Verbosity levels for sage-spkg wouldn't be bad either, especially suppressing the verbose tar output is IMHO worth doing, since a lot of spkgs contain hundreds or thousands of files (while the actual build log is often only a few lines).

I think removing the 'v' option from tar output would be sensible. It would make the log files smaller & reduce disk I/O. It would also make finding things like "warnings" easier, as you could grep the log looking for warnings, and not find every file which has the name "warning" in it.

In the absence of anyone writing a patch to control verbosity, just removing the 'v' would be a useful addition I think. The default action should not be to produce tons of unnecessary output.

Dave

comment:29 in reply to: ↑ 28 Changed 9 years ago by leif

Replying to drkirkby:

In the absence of anyone writing a patch to control verbosity, just removing the 'v' would be a useful addition I think. The default action should not be to produce tons of unnecessary output.

I can provide a patch for that. :) (Though there are currently a couple? of other tickets modifying sage-spkg.)

We can still fall back to the old behavoir if yet another undocumented environment variable (e.g. SAGE_SPKG_VERBOSE or SAGE_SPKG_LIST_FILES) is set, to not annoy people who heat their home with harddisks, or need more network traffic etc.

The advantage of verbose extraction is that you by chance see a lot of useless files that could or should be removed from an spkg. Take e.g. a look at the files in SageNB's Mercurial repository, currently also listed in spkg/logs/sagenb-* (it's about 22500 lines)... ;-)

comment:30 Changed 9 years ago by mpatel

I recently opened #10040 for not using tar's v option (by default) when extracting packages.

comment:31 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:32 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.6.1.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:33 Changed 9 years ago by mpatel

  • Milestone changed from sage-4.6 to sage-4.6.1

comment:34 Changed 9 years ago by leif

One more argument: ;-)

Successfully installed gnutls-2.2.1.p5
Now cleaning up tmp files.
Making Sage/Python scripts relocatable...
python: /home/leif/Sage/sage-4.6/local/lib/libz.so.1: no version information available (required by python)
Making script relocatable
Finished installing gnutls-2.2.1.p5.spkg
make[1]: Leaving directory `/home/leif/Sage/sage-4.6/spkg'

real    3m36.199s
user    10m5.250s
sys     2m8.140s
Error building Sage.
./sage -docbuild all html  2>&1 | tee -a dochtml.log
python: /home/leif/Sage/sage-4.6/local/lib/libz.so.1: no version information available (required by python)
python: can't open file '/home/leif/Sage/sage-4.6/devel/sage/doc/common/builder.py': [Errno 2] No such file or directory
. local/bin/sage-env && sage-starts && ./sage -tp 8 -sagenb -long devel/sage/doc/common devel/sage/doc/en devel/sage/doc/fr devel/sage/sage 2>&1 | tee -a ptestlong.log
python: /home/leif/Sage/sage-4.6/local/lib/libz.so.1: no version information available (required by python)
Testing that Sage starts...
python: /home/leif/Sage/sage-4.6/local/lib/libz.so.1: no version information available (required by python)
python: /home/leif/Sage/sage-4.6/local/lib/libz.so.1: no version information available (required by python)
python: /home/leif/Sage/sage-4.6/local/lib/libz.so.1: no version information available (required by python)
Traceback (most recent call last):
  File "/home/leif/Sage/sage-4.6/local/bin/sage-eval", line 4, in <module>
    from sage.all import *
ImportError: No module named sage.all
Sage failed to startup.
make: *** [ptestlong] Error 1
Note: See TracTickets for help on using tickets.