Opened 8 years ago

Closed 8 years ago

#11969 closed defect (fixed)

Clean up top-level Makefile

Reported by: jdemeyer Owned by: GeorgSWeber
Priority: major Milestone: sage-4.8
Component: build Keywords: Makefile pipestatus
Cc: Merged in: sage-4.8.alpha2
Authors: Jeroen Demeyer Reviewers: John Palmieri, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11926, #11972, #11959 Stopgaps:

Description (last modified by jdemeyer)

Using && inside commands for pipestatus does not work. Therefore, in the top-level Makefile, we should move $(TESTPRELIMS) out of $(PIPE).

For example, if sage-starts fails, one would get

$ make ptestlong
[...]
spkg/pipestatus ". local/bin/sage-env && sage-starts && ./sage -tp 0  -sagenb -long devel/sage/doc/common devel/sage/doc/de devel/sage/doc/en devel/sage/doc/fr devel/sage/doc/ru devel/sage/sage 2>&1" "tee -a ptestlong.log"

Testing that Sage starts...
Traceback (most recent call last):
  File "/usr/local/src/sage-4.7.2.alpha4/local/bin/sage-eval", line 4, in <module>
    from sage.all import *
  File "/usr/local/src/sage-4.7.2.alpha4/local/lib/python2.6/site-packages/sage/all.py", line 68, in <module>
    from sage.misc.all       import *         # takes a while
  File "/usr/local/src/sage-4.7.2.alpha4/local/lib/python2.6/site-packages/sage/misc/all.py", line 81, in <module>
    from functional import (additive_order,
  File "/usr/local/src/sage-4.7.2.alpha4/local/lib/python2.6/site-packages/sage/misc/functional.py", line 36, in <module>
    from sage.rings.complex_double import CDF
  File "integer.pxd", line 9, in init sage.rings.complex_double (sage/rings/complex_double.c:14716)
ImportError: libgmp.so.8: cannot open shared object file: No such file or directory
Sage failed to start up.
Please email sage-devel (http://groups.google.com/group/sage-devel)
explaining the problem and send the log file
  /usr/local/src/sage-4.7.2.alpha4/start.log
Describe your computer, operating system, etc.
spkg/pipestatus: line 44: [: -ne: unary operator expected
make: *** [ptestlong] Error 1

Note the penultimate line.

The attached patch also:

  1. removes sage-starts from sage -tp, because running make ptest or make ptestlong already runs sage-starts.
  2. runs ./sage -b in the make build rule. This ensures that the Sage library is up-to-date when testing or building documentation.
  3. adds make testalllong and make ptestalllong for completeness.
  4. generally cleans up the Makefile.

Apply:

  1. 11969_testprelims.patch to SAGE_ROOT
  2. 11969_scripts.patch to scripts

Attachments (2)

11969_scripts.patch (632 bytes) - added by jdemeyer 8 years ago.
11969_testprelims.patch (5.5 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 8 years ago by jdemeyer

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

comment:2 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 8 years ago by jdemeyer

  • Keywords Makefile pipestatus added

comment:4 Changed 8 years ago by leif

Looks reasonable, although you could just use parentheses in the parameter to pipestatus. (That way, the output of sage-starts would still get logged, too.)

comment:5 Changed 8 years ago by jhpalmieri

Looks okay to me, too. I think it also makes sense to move sage-env out of TESTPRELIMS and only run it explicitly where needed. I think that logging the output from sage-starts might be nice, but it's pretty minor.

Does sage -t accept --optional as meaning the same as -optional? It accepts --sagenb and --long. Do you want to change those while you're editing those lines?

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

  • Status changed from needs_review to needs_work

I noticed that make ptestlong actually runs sage-starts twice: once from the Makefile and once from sage-sage.

I propose not to run sage-starts when the user explicitly runs

$ ./sage -tp [...]

but to run sage-starts only when the user does

$ make ptestlong

The rationale is that sage -tp is often done with one or few files and then the overhead of sage-starts might be significant. However, for a make ptestlong run, running sage-starts once does not really impact the testing time.

Changed 8 years ago by jdemeyer

comment:7 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:8 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

I made some additional changes to the top-level Makefile, needs_review

comment:10 Changed 8 years ago by jdemeyer

  • Description modified (diff)

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

I'm wary of not running sage-starts as part of "sage -tp". It was introduced in #7103 to fix a problem when someone ran "sage -tp" before actually running Sage. I can see someone doing "make build" and then "./sage -tp ..." in order to test whether a new Sage build fixes problems in a particular directory.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 8 years ago by jdemeyer

Replying to jhpalmieri:

I can see someone doing "make build" and then "./sage -tp ..." in order to test whether a new Sage build fixes problems in a particular directory.

Then all those tests would fail. Is that a problem?

In all seriousness: I can see some arguments supporting sage-starts before ./sage -tp [...]. But then we should also do that for ./sage -t [...]. I value consistency and predictability very high, I prefer either "always" or "never" to anything in between.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by jhpalmieri

Replying to jdemeyer:

Replying to jhpalmieri:

I can see someone doing "make build" and then "./sage -tp ..." in order to test whether a new Sage build fixes problems in a particular directory.

Then all those tests would fail. Is that a problem?

No, the problem is that you might get this:

A mysterious error (perhaps a memory error?) occurred, which may have crashed doctest.

And you get it from a race condition, where multiple processes are simultaneously trying to create the same subdirectories of .sage. So it doesn't occur when you do "sage -t", only "sage -tp".

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

Replying to jdemeyer:

I noticed that make ptestlong actually runs sage-starts twice: once from the Makefile and once from sage-sage.

I propose not to run sage-starts when the user explicitly runs

$ ./sage -tp [...]

but to run sage-starts only when the user does

$ make ptestlong

The rationale is that sage -tp is often done with one or few files and then the overhead of sage-starts might be significant. However, for a make ptestlong run, running sage-starts once does not really impact the testing time.

Yes, definitely, i.e. remove it from sage-sage.

comment:15 Changed 8 years ago by jdemeyer

  • Dependencies set to #11926

Rebased to #11926.

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

Replying to jhpalmieri:

And you get it from a race condition, where multiple processes are simultaneously trying to create the same subdirectories of .sage.

How is this solved by running sage-starts? I would assume that a user doing ./sage -tp has already ran Sage at least once. And it also seems that #11926 should solve precisely this problem (except in the rare case where the changed code has to do with creating directories in the Sage startup).

Also, such a race condition is a bug in Sage. If you have more information about when it occurs and for which directories, we can look into this. (I find it very annoying that Python doesn't seem to have an equivalent of mkdir -p, making a directory but not complaining if it exists).

comment:17 Changed 8 years ago by leif

Sorry, hadn't read John's comments before my last post.

But IMHO, if there are race conditions in creating directories (in $DOT_SAGE/), the race conditions should get fixed instead of using dumb work-arounds that are hardly ever necessary.

Note that e.g. only sage-ptest supports SAGE_TEST_ITER.

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

Replying to jdemeyer:

Replying to jhpalmieri:

And you get it from a race condition, where multiple processes are simultaneously trying to create the same subdirectories of .sage.

I would assume that a user doing ./sage -tp has already ran Sage at least once. And it also seems that #11926 should solve precisely this problem (except in the rare case where the changed code has to do with creating directories in the Sage startup).

This doesn't help if $DOT_SAGE differs, e.g. because the user changed the variable's setting, or it's just a different user that runs Sage [for the first time maybe, or deleted his previous .sage, or there are new directories to create].

comment:19 Changed 8 years ago by jhpalmieri

If you have more information about when it occurs and for which directories, we can look into this.

You could look at #7103, as I mentioned above, and to #7079, to which #7103 refers.

I find it very annoying that Python doesn't seem to have an equivalent of mkdir -p, making a directory but not complaining if it exists

See #11972. I think the patch there should also take care of the race conditions when creating DOT_SAGE and its various subdirectories, which seemed to be the problem with #7103. One way to test: remove the sage-starts code from the doctesting routines and set DOT_SAGE to a non-existing directory and run "sage -tp ...". Repeat many times and see if you ever get "mysterious errors". Then apply the patch at #11972 and see if that fixes it.

If this fixes it, then I'm willing to make #11972 a prerequisite for this ticket and remove the execution of sage-starts from sage-sage. But I'm not willing to remove sage-starts unless we're pretty sure that we haven't reintroduced a possible bug in doctesting.

comment:20 Changed 8 years ago by jdemeyer

  • Dependencies changed from #11926 to #11926, #11972

Changed 8 years ago by jdemeyer

comment:21 Changed 8 years ago by jdemeyer

  • Dependencies changed from #11926, #11972 to #11926, #11972, #11959

comment:22 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:23 Changed 8 years ago by jdemeyer

  • Summary changed from Move $(TESTPRELIMS) out of pipestatus to Clean up top-level Makefile

comment:24 Changed 8 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

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

  • Milestone set to sage-4.8
  • Reviewers set to John Palmieri, Leif Leonhardy
  • Status changed from needs_review to positive_review

This all looks good to me, and I'm happy with the patch to the scripts repo now that #11972 is merged.

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

Replying to jhpalmieri:

This all looks good to me, and I'm happy with the patch to the scripts repo now that #11972 is merged.

Thanks for the review!

comment:27 Changed 8 years ago by jdemeyer

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