Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#11926 closed defect (fixed)

"make" should run Sage once

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

Description (last modified by jdemeyer)

In a multi-user environment, the user compiling Sage must run it at least once to run sage-location and generate .pyc files.

The proposed fix is: in the default make rule, run Sage when local/bin/sage-started.txt does not exist and create that file in sage-location. Also run Sage after upgrading.

This patch also changes the error message when a spkg fails a build or test. Example error message:

Error: Configuring PARI with readline and GMP kernel failed.

real    0m0.100s
user    0m0.012s
sys     0m0.012s
************************************************************************
Error installing package pari-2.4.3.alpha.p7
************************************************************************
Please email sage-devel (http://groups.google.com/group/sage-devel)
explaining the problem and including the relevant part of the log file
  /usr/local/src/sage-4.7.2.rc0/spkg/logs/pari-2.4.3.alpha.p7.log
Describe your computer, operating system, etc.
If you want to try to fix the problem yourself, *don't* just cd to
/usr/local/src/sage-4.7.2.rc0/spkg/build/pari-2.4.3.alpha.p7 and type 'make' or whatever is appropriate.
Instead, the following commands setup all environment variables
correctly and load a subshell for you to debug the error:
  (cd '/usr/local/src/sage-4.7.2.rc0/spkg/build/pari-2.4.3.alpha.p7' && '/usr/local/src/sage-4.7.2.rc0/sage' -sh)
When you are done debugging, you can type "exit" to leave the subshell.
************************************************************************
Error: Failed to install package 'pari'.

Apply:

  1. 11926.patch to SAGE_ROOT
  2. 11926_sage_starts.patch, trac_11926-error-msg.patch, 11926-error-msg-review.patch to SCRIPTS (local/bin)
  3. 11926_sage.patch and 11926_doc.patch to the Sage library.

Attachments (6)

trac_11926-error-msg.patch (3.4 KB) - added by jhpalmieri 9 years ago.
scripts repo; apply on top of other scripts patch
11926-error-msg-review.patch (2.6 KB) - added by jdemeyer 9 years ago.
11926_sage_starts.patch (4.8 KB) - added by jdemeyer 9 years ago.
Apply to SCRIPTS repository
11926.patch (1.4 KB) - added by jdemeyer 9 years ago.
Apply to the SAGE_ROOT repository
11926_sage.patch (3.8 KB) - added by jdemeyer 9 years ago.
11926_doc.patch (1.6 KB) - added by jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (98)

comment:1 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 9 years ago by jdemeyer

  • Status changed from new to needs_review

comment:3 follow-ups: Changed 9 years ago by jhpalmieri

I think I suggested this sort of thing once, and it was criticized. I don't remember the critiques. Anyway, I like the idea. In case some system administrator is building Sage and doesn't want a .sage directory created for them, should we set DOTSAGE to something temporary first?

By the way, running Sage as part of the build process should also take care of some of the issues at #5155 (not all of them, unfortunately).

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

  • Status changed from needs_review to needs_work
  • Work issues set to temporary DOTSAGE

Replying to jhpalmieri:

I think I suggested this sort of thing once, and it was criticized. I don't remember the critiques. Anyway, I like the idea.

In case some system administrator is building Sage and doesn't want a .sage directory created for them, should we set DOTSAGE to something temporary first?

Good idea. Make a temporary DOTSAGE, run Sage, remove the temporary DOTSAGE

By the way, running Sage as part of the build process should also take care of some of the issues at #5155 (not all of them, unfortunately).

Thanks for the pointer. Also #11887 is related.

comment:5 follow-up: Changed 9 years ago by leif

I don't like the idea of running sage after make build since it certainly doesn't belong to that target, but if at all, it shouldn't be run from spkg/install but the Makefile, preferably in the default rule, not build. Or run sage-location from there if make did anything, but we should in principle run it from elsewhere.

Someone who is going to install Sage system-wide should run make ptestlong or similiar anyway (which also runs sage), just like one runs make check before running make install.

Also note that sage (or sage-location) should be run (at least) after it has been moved to its final installation location, which is likely to happen after having built Sage.

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

Replying to leif:

I don't like the idea of running sage after make build since it certainly doesn't belong to that target

I think it does. Currently, after make build (or simply make), Sage does not work for a different user. I think make build should result in a proper Sage installation, which means that it should work for every user.

it shouldn't be run from spkg/install but the Makefile

Why? I like spkg/install better because it allows us to run Sage only if something was actually installed. You don't want to run Sage everytime somebody does make foo for various values of "foo". Also, IIRC, we run spkg/install on upgrading but not a top-level make all or make build.

Or run sage-location from there if make did anything, but we should in principle run it from elsewhere.

Running sage-location is certainly not sufficient to generate all files needed to run Sage.

Someone who is going to install Sage system-wide should run make ptestlong

Let the sysadmin decide whether they want to run make ptestlong or not, I want to give him the option of NOT doing it.

Also note that sage (or sage-location) should be run (at least) after it has been moved to its final installation location, which is likely to happen after having built Sage.

I would think most people run Sage from the location where they installed it, or equivalently, install Sage in the location they want to run it from. In any case, this does not invalidate the reasons for this ticket.

comment:7 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review
  • Work issues temporary DOTSAGE deleted

Updated version of the patch using a temporary DOT_SAGE directory. I have not really tested it well.

comment:8 Changed 9 years ago by jdemeyer

Tested with a complete build from scratch that this does indeed what it should do. A simple make build allows any user to run Sage.

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

I don't see anything in the new patch about DOT_SAGE.

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

Replying to jhpalmieri:

I don't see anything in the new patch about DOT_SAGE.

So true! I seems I forget a hg qrefresh.

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

Replying to jhpalmieri:

In case some system administrator is building Sage and doesn't want a .sage directory created for them, should we set DOTSAGE to something temporary first?

Related question which comes up: should we also use a temporary DOT_SAGE directory for make doc? In #10163, I found out that make doc does run Sage (and does create a DOT_SAGE directory).

comment:12 Changed 9 years ago by jdemeyer

  • Description modified (diff)

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

I'm somewhat tempted to add a new command-line option to Sage:

  • sage-sage

    diff --git a/sage-sage b/sage-sage
    a b if [ $# -gt 0 ]; then 
    247247  fi
    248248fi
    249249
     250# The following function creates a temporary file and stores its name
     251# in $FILE.  (It would be nice to replace this with 'mktemp', but that
     252# command has different syntax on OS X compared to linux or Solaris.)
     253sagetempfile() {
     254    FILE=`mktemp -d 2>/dev/null`
     255    if [ $? -ne 0 ]; then
     256        # presumably because the "-d" option to mktemp expects an argument,
     257        # e.g., on OS X.
     258        FILE=`mktemp -d -t dotsage`
     259    fi
     260}
     261
     262if [ "$1" = '--norc' -o "$1" = '--nodotsage' ]; then
     263    OLD_DOT_SAGE=$DOT_SAGE
     264    sagetempfile
     265    DOT_SAGE=$FILE && export DOT_SAGE
     266    shift
     267    sage "$@"
     268    rm -rf "$DOT_SAGE"
     269    DOT_SAGE=$OLD_DOT_SAGE && export DOT_SAGE
     270fi
    250271
    251272LOGFILE="$SAGE_ROOT/sage.log"
    252273LOGOPT=""

We should probably check exit codes for various things here, and of course this would need to be documented. If we had this, then in Makefile, we could replace "sage --docbuild ..." with "sage --norc --docbuild ...". We could possibly do the same with "make test", etc.

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

  • Status changed from needs_review to needs_work

Replying to jhpalmieri:

I'm somewhat tempted to add a new command-line option to Sage:

Let's discuss this in a different ticket: #11932.

I think I am going to revert the DOT_SAGE handling here and leave that for #11932, because currently it's very hard to avoid having a $HOME/.sage directory created when installing Sage.

comment:15 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:16 follow-ups: Changed 9 years ago by leif

s/succesfully/successfully/

Still, running sage (especially the full bunch of shell scripts) there is quite dangerous, as almost everything can happen. Worse, there's no "DO NOT INTERRUPT THIS"*, and error messages aren't even logged.

Better write some install target (and / or a sage-first_time script), and update the Sage Installation Guide, especially with respect to files that are created upon first start-up, hardcoded path etc. etc.

IMHO we should in general do certain things upon installing Sage (some actually during the build), rather than checking them and doing them for the first time when someone tries to start Sage.


  • This message is btw. printed too late, as install_moved() already does some stuff before the script (eventually!) gets to this.


P.S.: No serious sysadmin builds in, say, /usr/local/, or leaves an "installation" in /tmp/.

comment:17 Changed 9 years ago by leif

s/does not run/failed to start up/.

comment:18 follow-up: Changed 9 years ago by leif

Also, upon upgrading the message is useless (at least in the first place), as a second attempt to upgrade / build Sage is made.

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

Instead of running Sage, you could run sage-location and sage-python -c 'from sage.all import *', along with printing messages like "Generating necessary pyc files; do not interrupt". This should generate the pyc files and the other files in local/lib, right? This avoids the shell script sage-sage, but is there any real advantage to doing this instead of just running sage?

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

Replying to jhpalmieri:

Instead of running Sage, you could run sage-location and sage-python -c 'from sage.all import *', along with printing messages like "Generating necessary pyc files; do not interrupt". This should generate the pyc files and the other files in local/lib, right? This avoids the shell script sage-sage, but is there any real advantage to doing this instead of just running sage?

Not much I think. But we should really separate the things that have to (or should better) be done before running Sage the first time. Compiling Python files does IMHO not belong to this, and executing from sage.all import * can again do almost everything (or lead to arbitrary constellations if the build is broken), and is hardly guaranteed to at all exit by itself.

Creating a (fully working) "read-only" installation (from the perspective of an arbitrary user) is certainly not something that belongs to a regular build; you'd do such if you intend to install Sage system-wide for example of course, hence there should be some (separate) target for this. Such target could be the default target (all), although I think it shouldn't, at least not unless there's a (well-documented) way to specify the destination in advance, during some manually run configure for example.

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

On one hand, there is something attractive about building and installing Sage by running (perhaps) "configure", then "make", then "make install". However, this would require some work, and also a potential change of behavior for most Sage developers. This sounds like a possibility, but a long-term one. I don't see it happening any time soon, anyway. Jeroen's suggestion here, on the other hand, is a quick solution for a problem with Sage now. I suggest we merge this now (once the typos are fixed, along with any other simple fixes), and consider a longer term solution separately.

I wouldn't object if this were implemented by adding a new target in the Makefile, but I think it should be run by default to address the issues Jeroen has raised here. I also wouldn't object if it were implemented as in the current patch, perhaps with a followup ticket to make it a separate target for make.

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

Replying to jhpalmieri:

Jeroen's suggestion here, on the other hand, is a quick solution for a problem with Sage now.

I don't see any problem, except that the current behaviour of Sage regarding on-the-fly creation or modification of files in the "static" installation tree is suboptimal and -- IMHO worse -- AFAIK not documented, at least not very well.

And the "problem" is all but new.

Sysadmins -- at least those that have installed Sage before (others will quickly notice) -- are likely to be aware of that; most other users don't care at all, so this is rather a non-issue.

Adding running sage at the end of every (perhaps partial re-) build is not the right way to solve this; in contrast, it is likely to obscure errors that don't show up during the build but when you try to start Sage, in addition with the potential to make the build hang (without any output).

So the minimal things to do are -- if you really want to run Sage there:

  • Print a message that we're attempting to run sage.
  • Disable keyboard interrupts during important operations (sage-location, maybe elsewhere, too), and issue a warning that this process must not be interrupted, at least for a few minutes.
  • Perhaps set a reasonably long timeout for running sage.
  • Log the output instead of redirecting it to /dev/null, such that we can print at least a reference to the error log in case of failures.


I suggest we merge this now (once the typos are fixed, along with any other simple fixes), and consider a longer term solution separately.

There are various possible long-term solutions; the primary goal should be to document potential issues, i.e., at least prominently state that Sage should be run at least once at its final location for use in a multi-user environment, or more precisely, for a system-wide installation.


I wouldn't object if this were implemented by adding a new target in the Makefile, but I think it should be run by default to address the issues Jeroen has raised here.

An echo statement at the end of the build, perhaps in the Makefile, would IMHO fully suffice for the moment; no need to hurry or for "quick [and dirty] fixes" (which btw. often cause more trouble than they solve).

Have there been any "error" reports regarding this? Changing the permissions of all files in a couple of spkgs wasn't necessary either.

Note that "ordinary" users won't be able to install / upgrade spkgs anyway, so the pkg-config and .la files for example aren't an issue in a system-wide installation, nor will such users be able to move Sage, unless they copy the whole installation (in which case they're likely to gain ownership, or at least can change the owner, such that they'll have write permission on the copied installation).

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

Replying to leif:

Still, running sage (especially the full bunch of shell scripts) there is quite dangerous, as almost everything can happen.

What's your point really? Whoever built Sage will have to run it sooner or later, otherwise it's quite pointless building Sage. So the danger is there anyway.

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

Replying to leif:

Also, upon upgrading the message is useless (at least in the first place), as a second attempt to upgrade / build Sage is made.

What do you mean by "a second attempt to upgrade / build Sage is made"?

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

Replying to leif:

Better write some install target (and / or a sage-first_time script), and update the Sage Installation Guide, especially with respect to files that are created upon first start-up, hardcoded path etc. etc.

IMHO we should in general do certain things upon installing Sage (some actually during the build), rather than checking them and doing them for the first time when someone tries to start Sage.

In principle you are of course right but Sage is quite different from other software. It currently has no make install rule, and it is not at all clear to me what make install should do anyway. Also, Sage is special in that the source code is really in the installation. Normally, you would have a source tree and a build tree, but with Sage this is one. Sage developers are used to this, so people might even complain if you try to change this. So for this ticket, let's not overhaul the whole Sage build process, let's stick to the simple solution.

  • This message is btw. printed too late, as install_moved() already does some stuff before the script (eventually!) gets to this.

See #5155.

Leif: would you agree with the following: add a make install rule in Sage which simply runs Sage (and hence indirectly sage-location)? And then not run Sage from spkg/install.

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

Replying to jdemeyer:

Replying to leif:

Still, running sage (especially the full bunch of shell scripts) there is quite dangerous, as almost everything can happen.

Whoever built Sage will have to run it sooner or later, otherwise it's quite pointless building Sage. So the danger is there anyway.

:D I didn't know that. I only build Sage, but have never run it.

Seriously, the difference is whether a user intentionally runs sage (for the first time after a build / upgrade / extraction of a binary distribution), and sees what's happening. Your new patch omits the redirection to /dev/null, but I think it would be better to log the output to something like $SAGE_ROOT/first_sage_startup-`date +"%Y-%m-%d-%H:%M"`.log, which we could delete afterwards if apparently nothing went wrong, otherwise point the user to this file, e.g. for bug reports (also to sage-release btw.).

We have enough "error reports" where people claim something didn't work but couldn't provide error logs.


I'd also prefer running Sage from the Makefile; rather than changing the build target, we could modify the default target to run sage once after a successful build (or after building the documentation, but that's likely to fail in "weird" ways if sage itself doesn't work properly), perhaps just if no (recent) "first run" logfile exists.

There we should IMHO separate what's currently implicitly performed by sage-location, for example by adding an option to sage-sage for running just that. For upgrades, running sage-location afterwards is not strictly necessary; although some parts should be run by sage-spkg anyway, such that they also get run upon e.g. sage -i .... If SAGE_UPGRADING=yes, we could add a message to spkg/install (or sage-sage, or sage-update) recommending to try to run sage right now (provided the build succeeded). make upgrade (and make rebuild) are overdue anyway; if/when we drop the creation of pipestatus, we could also change sage-update to just download new files and then run make rebuild, i.e., make it use the top-level Makefile as well (which is now under revision control and hence subject to upgrades, too).

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

Replying to jdemeyer:

Replying to leif:

Also, upon upgrading the message is useless (at least in the first place), as a second attempt to upgrade / build Sage is made.

What do you mean by "a second attempt to upgrade / build Sage is made"?

if [ "$1" = '-upgrade' -o "$1" = "--upgrade" ]; then
    # People often move the Sage install right before doing the upgrade, so it's
    # important to fix any path hardcoding issues first, or certain library
    # links will fail.
    "$SAGE_ROOT/local/bin/"sage-location

    # Do it twice since when installing sage-scripts and a running
    # script changes, it gets confused and exits with an error.
    # Running again (with the script replaced) then fixes the problem.
    # Run from a temporary copy of sage-sage
    shift
    sage-upgrade "$@"
    if [ $? = 2 ]; then   # this exit codes means the user elected not to do the upgrade at all.
        exit $?
    fi
    echo "Double checking that all packages have been installed."
    sage-upgrade "$@"
    exit $?
fi

comment:28 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:29 Changed 9 years ago by jdemeyer

Updated version, needs review (even though I have not yet tested building from scratch).

comment:30 Changed 9 years ago by jdemeyer

  • Keywords sage-starts added

comment:31 Changed 9 years ago by jdemeyer

Any reviewers for this quite simple patch?

comment:32 Changed 9 years ago by jhpalmieri

I haven't tested it out, but it looks good at first glance. If you're going to make changes to the error messages printed by sage-spkg, it would be good to mention $SAGE_ROOT/spkg/logs/$PKG_NAME.log. If people build in parallel (and I hope they do), $SAGE_ROOT/install.log can be a real mess, and so may not be the best way to extract information. What do you think about my attached patch?

Changed 9 years ago by jhpalmieri

scripts repo; apply on top of other scripts patch

comment:33 Changed 9 years ago by jhpalmieri

  • Description modified (diff)

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

I think that in sage-starts, if Sage fails to start, you should delete SAGE_ROOT/local/bin/sage-flags.txt: this is created by sage-location, which gets run by sage, which gets run by sage-starts. That is, the file sage-flags.txt might be created successfully, then

spkg/pipestatus "./sage -c \"print 'Yes, Sage starts.'\" 2>&1" "tee -a start.log"

fails, and then if you run "make" again, it won't test whether Sage starts because of the presence of sage-flags.txt.

Either that, or there needs to be a different test in the Makefile to decide whether to run sage-starts.

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

  • Status changed from needs_review to needs_work

Replying to jhpalmieri:

Either that, or there needs to be a different test in the Makefile to decide whether to run sage-starts.

How about having a third file besides the existing sage-flags.txt and sage-current-location.txt? Say, a file sage-started.txt whose existence means Sage was run at least once successfully.

I think we could also use the non-existence of this file as a trigger in sage-location to force some actions. For example, on upgrading, even if the Sage tree was not moved, it might make sense to call update_library_files() and update_pkgconfig_files().

I would write the file sage-started.txt in sage-location and potentially delete it in sage-starts (that's what you proposed for sage-flags.txt instead).

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

You could instead test whether the file starts.log ends with "Yes, Sage starts." You could test this in sage-starts rather than in the Makefile.

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

Replying to jhpalmieri:

You could instead test whether the file starts.log ends with "Yes, Sage starts."

Sorry, but I disagree. I think neither the existence nor contents of any logfiles should influence the runtime at all. Users should be able to do whatever they want with logfiles without it making any difference.

comment:38 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to John Palmieri
  • Status changed from needs_work to needs_review

comment:39 Changed 9 years ago by jdemeyer

New patches up for review. It uses a new file local/lib/sage-started.txt to check whether to run sage-starts. I also added a reviewer patch for your changed error message.

comment:40 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:41 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:42 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:43 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:44 Changed 9 years ago by jdemeyer

  • Description modified (diff)

Changed 9 years ago by jdemeyer

comment:45 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:46 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from "make build" should run Sage once to "make" should run Sage once

comment:47 follow-up: Changed 9 years ago by leif

For the record:

Installation in a Multiuser Environment
---------------------------------------

This section addresses the question of how a system administrator
can install a single copy of Sage in a multi-user computer
network.

System-wide install
~~~~~~~~~~~~~~~~~~~

#. After you build Sage, you may optionally copy or move the entire
   build tree to ``/usr/local``.

#. There are some initial files that have to be created the first time
   Sage is run. Try starting up Sage once as root (or, to be
   more thorough, try ``make test`` as root to run all the standard test
   code). You can stop the tests by pressing ``ctrl-z`` followed by
   typing ``kill %1`` (assuming you had no other jobs in the
   background of that shell).

...

(I.e., RTFM.)

comment:48 follow-up: Changed 9 years ago by leif

The patch to spkg/install will conflict with #11959, so should be removed from the patch. (The changes are irrelevant anyway.)

comment:49 follow-up: Changed 9 years ago by leif

I don't think it's a good idea to write the "sage started" file from sage-location, as running sage-location doesn't imply that Sage will or will get run.

Btw., .pyc wasn't a typo; while the .pyc files don't get "regenerated" by sage-location, they get deleted, such that they'll indeed get regenerated the next time Sage is run.

(The .pc files don't get regenerated either; if at all, they get updated when necessary.)

comment:50 follow-ups: Changed 9 years ago by leif

The patch to sage-spkg is also unrelated (and of just cosmetic nature) and will just cause unnecessary merge conflicts.

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

Replying to leif:

The patch to sage-spkg is also unrelated (and of just cosmetic nature) and will just cause unnecessary merge conflicts.

s/the patch/the three patches/ !?!

These should better be split off and moved to (and later rebased on) #11021.

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

Replying to leif:

(I.e., RTFM.)

I think we should Edit The Fucking Manual then.

comment:53 Changed 9 years ago by jdemeyer

  • Description modified (diff)

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

Replying to leif:

The patch to spkg/install will conflict with #11959, so should be removed from the patch.

Done.

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

Replying to leif:

The patch to sage-spkg is also unrelated (and of just cosmetic nature) and will just cause unnecessary merge conflicts.

I agree it is unrelated and of cosmetic nature but it improves the error messages which is quite important I think. And #11021 is a ticket which has no activity since 3 months so I don't think moving it there is such a good solution.

Changed 9 years ago by jdemeyer

Apply to SCRIPTS repository

comment:56 Changed 9 years ago by jdemeyer

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

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

  • Status changed from needs_work to needs_review

Replying to leif:

I don't think it's a good idea to write the "sage started" file from sage-location, as running sage-location doesn't imply that Sage will or will get run.

Okay, moved this to sage/all.py, surely a better place. Needs review.

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

If someone runs 'make build' instead of 'make' or 'make start', is there a good way to let them know that they should probably run Sage once to initialize various files, or should we just hope that they've read enough of README.txt or the installation guide?

comment:59 in reply to: ↑ 58 ; follow-up: Changed 9 years ago by jdemeyer

Replying to jhpalmieri:

If someone runs 'make build' instead of 'make' or 'make start', is there a good way to let them know that they should probably run Sage once to initialize various files, or should we just hope that they've read enough of README.txt or the installation guide?

Well, we could print a message after make build is finished, but this will also be printed when doing a simple make and might confuse users. So I would rather not print anything and assume that users who know about make build also know what they are doing.

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

  • Reviewers changed from John Palmieri to John Palmieri, Leif Leonhardy
  • Status changed from needs_review to positive_review

I think this looks good. There are a few small cosmetic changes; if you want to make them, no need for re-review.

  • After having rewritten the error message in trac_11926-error-msg.patch myself, I now think it would look better to add blank lines above and below line 56, the name of the log file. (That's line 59 in 11926-error-msg-review.patch). The same might go for line 65 (in the review patch), the commands to try to debug the error, but it's not as important there.

Changed 9 years ago by jdemeyer

Apply to the SAGE_ROOT repository

Changed 9 years ago by jdemeyer

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

Replying to jhpalmieri:

In 11926_sage.patch, line 859, I would change "such that" to "so that".

Done

After having rewritten the error message in trac_11926-error-msg.patch myself, I now think it would look better to add blank lines above and below line 56, the name of the log file. (That's line 59 in 11926-error-msg-review.patch). The same might go for line 65 (in the review patch), the commands to try to debug the error, but it's not as important there.

Well, I think the indentation already makes them stand out. Looking at the new error message in the description of this ticket, I quite like it.

comment:62 Changed 9 years ago by jdemeyer

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

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

Replying to jdemeyer:

Replying to jhpalmieri:

If someone runs 'make build' instead of 'make' or 'make start', is there a good way to let them know that they should probably run Sage once to initialize various files, or should we just hope that they've read enough of README.txt or the installation guide?

Well, we could print a message after make build is finished, but this will also be printed when doing a simple make and might confuse users. So I would rather not print anything and assume that users who know about make build also know what they are doing.

ifeq ($(MAKECMDGOALS),build)
        RUN_SAGE_REMINDER = echo "Make sure to run Sage once before ..."
endif

...

build: ...
        ...
        $(RUN_SAGE_REMINDER)

IMHO this ticket still needs work.

First of all, doc should also depend on start[sic], to avoid attempting to run both at the same time.

Second, the output of sage-starts still isn't logged, which is important for error reports at least.

comment:64 in reply to: ↑ 63 ; follow-up: Changed 9 years ago by jdemeyer

Replying to leif:

First of all, doc should also depend on start[sic], to avoid attempting to run both at the same time.

Nothing wrong with running doc and start[sic] at the same time.

Second, the output of sage-starts still isn't logged

That's not true. It is logged in $SAGE_ROOT/start.log, see the new local/bin/sage-starts. If sage-starts fails, you get an error message pointing to this logfile.

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

Replying to leif:

Second, the output of sage-starts still isn't logged, which is important for error reports at least.

Ok, I see it is in $SAGE_ROOT/start.log. Not immediately clear from the Makefile, and having it in install.log as well wouldn't be bad either.

From start.log alone (which is always appended to), it isn't clear what happened inbetween (i.e., the relevant parts of install.log), and conversely, from install.log one doesn't necessarily see whether running Sage succeeded. Having the date and time (UTC) there (in start.log, but in general in install.log, too) wouldn't be bad either.

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

Replying to jdemeyer:

Replying to leif:

First of all, doc should also depend on start[sic], to avoid attempting to run both at the same time.

Nothing wrong with running doc and start[sic] at the same time.

Seriously? If both get run at the same time, the [screen] output is likely to get messed up (the message of sage-starts almost certainly not being the last). But more important, it doesn't make sense to try to build the documentation if Sage doesn't start up (since it will fail at some later point), and are you 100% sure that there aren't any further race conditions between these two?

comment:67 follow-up: Changed 9 years ago by leif

In the patch to sage-spkg (which is unrelated to this ticket anyway), error_msg() prints "sage: An error occurred while testing $PKG_NAME. regardless of $1.

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

Replying to leif:

In the patch to sage-spkg (which is unrelated to this ticket anyway), error_msg() prints "sage: An error occurred while testing $PKG_NAME. regardless of $1.

I see. Fixed in yet another reviewer patch... Sorry for the noise, although the patch is still unrelated to the ticket's topic. (More precisely, I already made similar changes at / for #11021, where it IMHO belongs.)

comment:69 follow-up: Changed 9 years ago by leif

W.r.t. the Installation Guide:

One shouldn't edit /usr/local/sage-*/sage (after copying it; one can of course), but /usr/local/bin/sage, i.e., one should either edit the copy, or the original and copy it afterwards.

No need to remove the recommendation to run make test or whatever (make ptestlong would IMHO be more appropriate).

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

Replying to leif:

W.r.t. the Installation Guide:

One shouldn't edit /usr/local/sage-*/sage (after copying it; one can of course), but /usr/local/bin/sage, i.e., one should either edit the copy, or the original and copy it afterwards.

P.S.: One should preferably edit the copy, since the original is under revision control, so changes to that can e.g. again cause "errors" during upgrading.

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

Replying to leif:

I see. Fixed in yet another reviewer patch... Sorry for the noise, although the patch is still unrelated to the ticket's topic. (More precisely, I already made similar changes at / for #11021, where it IMHO belongs.)

I don't see why it belongs more to #11021 than here, but in any case, that's not a very relevant discussion. I was unaware of #11021 when I made the changes and I'm not sure whether that ticket is ready to be merged.

comment:72 Changed 9 years ago by jdemeyer

  • Resolution fixed deleted
  • Status changed from closed to new

comment:73 Changed 9 years ago by jdemeyer

  • Status changed from new to needs_review

comment:74 Changed 9 years ago by jdemeyer

Added an extra patch with the documentation changes proposed by leif, needs_review.

comment:75 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:76 Changed 9 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:77 Changed 9 years ago by jdemeyer

  • Merged in changed from sage-4.7.3.alpha0 to sage-4.8.alpha0
  • Milestone set to sage-4.8

Changed 9 years ago by jdemeyer

comment:78 Changed 9 years ago by jdemeyer

Can somebody please review the last patch here?

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

  • Status changed from needs_review to positive_review

Looks okay to me.

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

Replying to jhpalmieri:

Looks okay to me.

Yes.

But still doc should depend on start, such that one immediately gets a single, meaningful error message, not intermixed by repeated random tracebacks from Sphinx (after each of which the script btw. still unconditionally prints "Build succeeded. The built documents can be found in...").

And it would be far better to have the output of sage-starts also appended to install.log, for the reasons given earlier.


W.r.t. #11021:

Despite its current title, it involves a "major" rewrite of sage-spkg, i.e., makes changes all over the code, half of which were -- though already positively reviewed -- removed from another ticket for IMHO no reason (since at that point, they did apply and didn't break any other tickets).

So that ticket certainly drags (which mainly is my fault; I've returned to it many times, but then again had to work on too many other tickets), but it doesn't make things easier if other tickets do redundant changes to sage-spkg, thereby just causing tedious rebasing.

One can by the way search for other tickets before working on e.g. scripts or spkgs. And there's btw. also a meta-ticket for changes to the Makefile, #11622.

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

Replying to leif:

But still doc should depend on start, such that one immediately gets a single, meaningful error message

I don't find it logical that doc would depend on start. I still think the best solution would be to have make build run sage-starts. So here, we have to agree to disagree.

So that ticket certainly drags (which mainly is my fault; I've returned to it many times, but then again had to work on too many other tickets), but it doesn't make things easier if other tickets do redundant changes to sage-spkg, thereby just causing tedious rebasing.

If we are not allowed to make patches just because some other ticket makes changes to the same code, then we might as well stop Sage development. Especially if the ticket gets "dragged" as you say.

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

  • Resolution set to fixed
  • Status changed from positive_review to closed

Replying to leif:

And it would be far better to have the output of sage-starts also appended to install.log, for the reasons given earlier.

I don't have strong feelings about this issue. If you want to make this change on a different ticket, go ahead, I will have a look.

comment:83 Changed 9 years ago by leif

Replying to jdemeyer:

Replying to leif:

But still doc should depend on start, such that one immediately gets a single, meaningful error message

I don't find it logical that doc would depend on start. I still think the best solution would be to have make build run sage-starts. So here, we have to agree to disagree.

The builder imports sage.all, hence doc requires that Sage can start.

Perhaps sage --docbuild should check once that it is functional (with try: import sage.all ...).


If we are not allowed to make patches just because some other ticket makes changes to the same code, then we might as well stop Sage development. Especially if the ticket gets "dragged" as you say.

I was talking about unnecessary, unrelated changes. Any developer should at least be aware of other tickets touching the same code.

It's also bad practice to bundle unrelated changes to different files into a single patch, just because they're in the same repo.

comment:84 Changed 9 years ago by jhpalmieri

See #11991 for what I hope is a quick a follow-up: sage-starts should record the time and version number.

comment:85 follow-up: Changed 9 years ago by fbissey

  • Cc pcpa added

I hate to be late to the party. I was bogged down into work. I appreciate the point of view that most people won't build the stuff system wide. Me and Paulo from Mandriva do. I and probably him will object to that sage-started.txt file being stored in $SAGE_LOCAL/lib/. To us it belongs to DOTSAGE. I know it complicates the testing that Jeroen wants to do. But this goes a long way to making a system wide installation troublesome.

I may have missed it but where is the documentation for someone who wants to make a system wide install? Just this ticket?

comment:86 Changed 9 years ago by jhpalmieri

I think that sage_started.txt should be viewed as a file like sage-current-location.txt: a file which is produced during the build/test process, which ordinary users don't need write-access to. This ticket is related to some of the issues at #5155, and should indeed fix some of them. But maybe I don't understand your complaint; can you explain it more?

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

Replying to fbissey:

I hate to be late to the party. I was bogged down into work. I appreciate the point of view that most people won't build the stuff system wide. Me and Paulo from Mandriva do. I and probably him will object to that sage-started.txt file being stored in $SAGE_LOCAL/lib/. To us it belongs to DOTSAGE. I know it complicates the testing that Jeroen wants to do. But this goes a long way to making a system wide installation troublesome.

Why do you think this makes a system-wide install harder? In fact, this ticket was exactly supposed to make it easier to do a system-wide install.

I may have missed it but where is the documentation for someone who wants to make a system wide install? Just this ticket?

See http://sagemath.org/doc/installation/source.html#installation-in-a-multiuser-environment. Of course this ticket (and probably others to be merged in sage-4.8) will change this documentation, but the essence should remain the same.

comment:88 follow-up: Changed 9 years ago by fbissey

OK. I just completed my first ebuild of a sage-4.8 alpha this afternoon. The first I noticed about this is when I ran the doctests as a normal user. I do that straight away as this is the only validation test I have. A truckload of doctests just failed complaining that they couldn't find "sage-started.txt". Actually so many failed that I am almost wondering if there is something wrong with the ones that didn't.

I spent 3 weeks fixing bad fortran code and bad makefiles so I may have had a bit of a temper there but it just made my life difficult.

I am not sure how it helps with #5155 since it adds stuff that actually wants access to SAGE_ROOT.

I'll admit that I skimmed a bit on reading this thread so I won't attribute bad intent but using DOT_SAGE has been raised and apparently shot down as "system wide install" is not a normal install. You don't have to support the kind of stuff I do but it felt like making it more difficult knowingly.

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

How did you build sage? i.e. which shell commands did you execute, starting from the Sage source tarball, to get the totally-non-working Sage?

comment:90 Changed 9 years ago by jdemeyer

Also: precisely which alpha version of Sage did you use? There have been various changes to the build process in the 4.8 series.

comment:91 Changed 9 years ago by jhpalmieri

I tried the following:

  • build Sage ('make'), version 4.8.alpha2
  • move the Sage installation and run 'sage' once, as per the multi-user installation instructions
  • chmod a-w -R sage-x.y.z (so I don't have write access anymore)
  • sage -tp 18 devel/sage

Almost all tests passed; the only failures were the ones reported at #5515.

The following tests failed:

	sage -t --long devel/sage/sage/modular/hecke/submodule.py # 1 doctests failed
	sage -t --long devel/sage/sage/modular/abvar/abvar.py # 1 doctests failed
	sage -t --long devel/sage/sage/lfunctions/sympow.py # 13 doctests failed
	sage -t --long devel/sage/sage/interfaces/qepcad.py # 3 doctests failed
	sage -t --long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 17 doctests failed

So I think the approach on this ticket should work with a multi-user installation, but as I said earlier, I may be misunderstanding the problem.

comment:92 Changed 9 years ago by fbissey

Sorry not to reply immediately Jeroen but that last message was the last thing I did before getting some sleep.

Which helped clear my mind a little and take on board some comments properly. I, and am pretty sure that Mandriva does too, bypass the normal sage build system in our packages. You may have noticed the word ebuild (gentoo). So I didn't notice that you basically generated a file that was essential to start sage at the very end of the building process and outside of any spkg (that's a very crucial bit if you are packaging sage). I guess now that I have noticed and realized it is supposed to be there I could just add it and adjust the location.

My real mistake was to think it was created each time sage was launched - never mind that there is no process to remove it.

Note: See TracTickets for help on using tickets.