Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#10494 closed defect (fixed)

Upgrading 4.6->4.6.1 does not upgrade sagenb

Reported by: jdemeyer Owned by: GeorgSWeber
Priority: blocker Milestone: sage-4.6.1
Component: build Keywords: sagenb
Cc: leif Merged in: sage-4.6.1.rc1
Authors: Leif Leonhardy, Jeroen Demeyer Reviewers: Jeroen Demeyer, Leif Leonhardy, Geoffrey Ehrman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

It seems that sage -f sagenb-VERSION doesn't actually do anything. It does not change the devel/sagenb-main directory. This issue also breaks upgrading sage-4.6 to sage-4.6.1.alpha3 for me, because sagenb is not actually upgraded.

Attachments (4)

10494.patch (10.8 KB) - added by jdemeyer 9 years ago.
SAGENB patch
10464_reviewer.patch (5.6 KB) - added by jdemeyer 9 years ago.
Reviewer patch, SAGENB
10494_reviewer.patch (5.2 KB) - added by jdemeyer 9 years ago.
Reviewer patch, SAGENB (fixed filename, ignore previous)
10494_reviewer2.patch (1.6 KB) - added by jdemeyer 9 years ago.
Second reviewer patch, SAGENB, apply on top of previous

Download all attachments as: .zip

Change History (18)

Changed 9 years ago by jdemeyer

SAGENB patch

comment:1 Changed 9 years ago by jdemeyer

  • Authors set to Leif Leonhardy
  • Cc leif added
  • Status changed from new to needs_review

Patch copied from #10176.

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

Leif, it seems your patch (unintentionally?) fixes upgrading.

comment:3 Changed 9 years ago by jdemeyer

I discovered what went wrong with upgrading: the command

cp -pr sagenb "$SAGE_ROOT/devel/sagenb-main"

would create a directory $SAGE_ROOT/devel/sagenb-main/sagenb if sagenb-main already exists. The patch fixes this by first renaming $SAGE_ROOT/devel/sagenb-main

comment:4 Changed 9 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:5 Changed 9 years ago by jdemeyer

Positive review to Leif's patch, added still untested reviewer patch.

Changed 9 years ago by jdemeyer

Reviewer patch, SAGENB

Changed 9 years ago by jdemeyer

Reviewer patch, SAGENB (fixed filename, ignore previous)

comment:6 Changed 9 years ago by jdemeyer

  • Merged in set to sage-4.6.1.rc0

comment:7 Changed 9 years ago by leif

  • command -v sage is a bit faster than sage -v, but I don't mind. (The latter isn't much more specific since other programs called sage are likely to also understand that option, with arbitrary results...)
  • Solaris' grep does understand -q, but unfortunately not the one in the "default" path. We should IMHO check in Sage's configure (and/or sage-env) that we have the proper / capable (POSIX-conformant) tools in the path, not limited to grep.
  • If sage -pkg ... fails, the error message should start with "Error: " and go to sys.stderr; I would also quote the failed command there.
  • W.r.t.
    # This spkg-dist doesn't need any Sage components, it is fine to run 
    # it from a system-wide Python. -- Jeroen Demeyer
    
    I wouldn't rely on a (potentially) system-wide python, since that might actually lack some components, or have versions which are too old (which definitely can cause import errors).

It's not that clear what you mean by "Sage components"; spkg-dist certainly requires Sage's Mercurial, some scripts and the programs these use. It also requires (at least) setuptools, which are shipped with Sage, but may not be installed in a system-wide Python.

(We could try: import ... except ImportError: ... to give a more "friendly" error message in the latter case. I don't know if some / what minimal versions of Python and the imported modules are required. Experienced Sage developers are likely to have current versions installed, but I think SageNB is meanwhile subject to changes by a wider group as well.)

So it would be safer to use Sage's Python; if we keep sage-python in SAGE_ROOT, which should be in the path, or at least sage-python itself, we could use

#!/usr/bin/env sage-python

in the spkg-dist script.

I think I'm ok with the rest of the reviewer patch; I still have to provide a version of my patch(es) with the currently "lost" commit messages...

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

Replying to jdemeyer:

Leif, it seems your patch (unintentionally?) fixes upgrading.

Of course intentionally. ;-)

Besides that a missing #! can have strange "side" effects, too, I replaced the cp by mv, which makes the deletion of sagenb-main obsolete (and is of course much faster).

comment:9 Changed 9 years ago by jdemeyer

Second reviewer patch added, needs review.

Changed 9 years ago by jdemeyer

Second reviewer patch, SAGENB, apply on top of previous

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

Possible issue: is it reasonable to expect SAGE_SETUPTOOLS_DEBUG to always be set to "yes" or is it worth worrying about something like SAGE_SETUPTOOLS_DEBUG == "Yes"?

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

  • Authors changed from Leif Leonhardy to Leif Leonhardy, Jeroen Demeyer
  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Leif Leonhardy

Replying to gbe:

Possible issue: is it reasonable to expect SAGE_SETUPTOOLS_DEBUG to always be set to "yes" or is it worth worrying about something like SAGE_SETUPTOOLS_DEBUG == "Yes"?

I think it is well-known that one should only use "yes" and no other variant, so I would not worry about this.

comment:12 Changed 9 years ago by gbe

  • Reviewers changed from Jeroen Demeyer, Leif Leonhardy to Jeroen Demeyer, Leif Leonhardy, Geoff Ehrman
  • Status changed from needs_review to positive_review

Looks good. The only other issue I see is some code in spkg-dist is never reached, but I'll open a separate ticket for that issue.

comment:13 Changed 9 years ago by jdemeyer

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

comment:14 Changed 9 years ago by jdemeyer

  • Reviewers changed from Jeroen Demeyer, Leif Leonhardy, Geoff Ehrman to Jeroen Demeyer, Leif Leonhardy, Geoffrey Ehrman
Note: See TracTickets for help on using tickets.