Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#13123 closed task (fixed)

Move SAGE_DATA to SAGE_LOCAL/share

Reported by: ohanar Owned by: tdb
Priority: major Milestone: sage-5.4
Component: relocation Keywords:
Cc: kini, jdemeyer, robertwb, burcin Merged in: sage-5.4.beta2
Authors: R. Andrew Ohana, Jeroen Demeyer Reviewers: François Bissey, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13457 Stopgaps:

Description (last modified by jdemeyer)

This is part of the new layout for git. Since it is not too difficult to separate off this change, I decided making this a distinct ticket was a good idea. SAGE_EXTCODE will be moved to SAGE_ROOT/devel/ext, and currently SAGE_LOCAL/share/sage/ext links to this.

New SPKGS:

Installation Steps:

Attachments (11)

elliptic_curves.diff (1.8 KB) - added by ohanar 7 years ago.
for review purposes
polytopes_db.diff (1.0 KB) - added by ohanar 7 years ago.
for review purpose
conway_polynomials.diff (1.6 KB) - added by ohanar 7 years ago.
for review purposes
graphs.diff (998 bytes) - added by ohanar 7 years ago.
for review purposes
trac13123_extcode.patch (2.2 KB) - added by ohanar 7 years ago.
apply to extcode repo
trac13123_scripts.patch (7.1 KB) - added by ohanar 7 years ago.
apply to scripts repo
13123_extcode_review.patch (3.1 KB) - added by jdemeyer 7 years ago.
trac13123_library.patch (39.1 KB) - added by ohanar 7 years ago.
apply to sage library
13123_bdist.patch (769 bytes) - added by jdemeyer 7 years ago.
13123_root_review.patch (4.1 KB) - added by jdemeyer 7 years ago.
trac13123_root.patch (2.7 KB) - added by jdemeyer 7 years ago.
apply to root repo

Download all attachments as: .zip

Change History (80)

comment:1 Changed 7 years ago by ohanar

  • Authors set to R. Andrew Ohana
  • Cc jdemeyer robertwb added
  • Dependencies set to #12205

comment:2 Changed 7 years ago by ohanar

  • Description modified (diff)

comment:3 Changed 7 years ago by ohanar

  • Description modified (diff)

comment:4 Changed 7 years ago by kini

  • Description modified (diff)

comment:5 Changed 7 years ago by ohanar

  • Dependencies #12205 deleted
  • Description modified (diff)
  • Status changed from new to needs_review

Ok, everything should work now.

comment:6 Changed 7 years ago by ohanar

  • Description modified (diff)

comment:7 follow-ups: Changed 7 years ago by fbissey

  • Reviewers set to François Bissey

That will cause me a lot of work in sage-on-gentoo but overall that will make my life easier in the long term. I cannot comment on trac13123_extcode.patch and trac13123_scripts.patch as I don't know the code in there all that well. But for the rest:

  • I will need to check the spkg individually for correctness but I assume it will be OK it is the easy part.
  • trac13123_library.patch : I am spotting something that I should have reported as a bug a long time ago. In sage/interfaces/gap.py GAP_STAMP is created in $SAGE_LOCAL/bin. This is a no-no if sage is installed globally and accessible by many users. That means all users would need to be able to write to $SAGE_LOCAL/bin to use gap from sage. There is no reason why GAP_STAMP has to be stored there. It should be moved to $DOT_SAGE or possibly even $GAP_DIR. There is a similar problem with qepcad but your patch doesn't touch that particular bit.

comment:8 Changed 7 years ago by jdemeyer

Please make it clear in the ticket description which patches have to be applied where.

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

You should use cp -R instead of cp -r (like it was before).

You should deal with upgrading (probably in the new extcode installer): if SAGE_ROOT/data exists, then move SAGE_ROOT/data/extcode to SAGE_ROOT/devel/ext-main and delete the whole directory SAGE_ROOT/data.

comment:10 follow-up: Changed 7 years ago by jdemeyer

Why don't you

mkdir -p "$SAGE_DATA"

in spkg/install? That's easier than doing it in the various packages.

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

Replying to fbissey:

  • trac13123_library.patch : I am spotting something that I should have reported as a bug a long time ago. In sage/interfaces/gap.py GAP_STAMP is created in $SAGE_LOCAL/bin. This is a no-no if sage is installed globally and accessible by many users. That means all users would need to be able to write to $SAGE_LOCAL/bin to use gap from sage. There is no reason why GAP_STAMP has to be stored there. It should be moved to $DOT_SAGE or possibly even $GAP_DIR. There is a similar problem with qepcad but your patch doesn't touch that particular bit.

See #5155.

comment:12 in reply to: ↑ 10 Changed 7 years ago by ohanar

Replying to jdemeyer:

Why don't you

mkdir -p "$SAGE_DATA"

in spkg/install? That's easier than doing it in the various packages.

Maybe easier, but since I was bumping the versions of these spkgs anyways for upgrading purposes, I figured I would have them take care of their own installation, rather than relying upon the build scripts.

comment:13 in reply to: ↑ 7 Changed 7 years ago by ohanar

Replying to fbissey:

That will cause me a lot of work in sage-on-gentoo but overall that will make my life easier in the long term.

You should expect to see a number of these types of changes from me over the next few months. It should definitely simply things for you as the environment variables SAGE_DATA and SAGE_EXTCODE will now be globally respected in Sage.

comment:14 Changed 7 years ago by ohanar

  • Description modified (diff)

comment:15 in reply to: ↑ 9 Changed 7 years ago by ohanar

Replying to jdemeyer:

You should use cp -R instead of cp -r (like it was before).

You should deal with upgrading (probably in the new extcode installer): if SAGE_ROOT/data exists, then move SAGE_ROOT/data/extcode to SAGE_ROOT/devel/ext-main and delete the whole directory SAGE_ROOT/data.

Done and done.

comment:16 follow-up: Changed 7 years ago by fbissey

Should we deal with gap_stamp in this ticket or should I add it to #5155 and try to push its review for 5.1?

comment:17 in reply to: ↑ 16 Changed 7 years ago by ohanar

Replying to fbissey:

Should we deal with gap_stamp in this ticket or should I add it to #5155 and try to push its review for 5.1?

Since I'm already touching that line, I may as well do it here. If you do end up finishing #5155 in time for 5.1, I'll have to rebase anyway :). (This is under the assumption that this ticket doesn't make it into 5.1, which I feel like is a pretty safe assumption.)

comment:18 Changed 7 years ago by fbissey

Thanks. Yes considering that Jeroen wants beta5 to be the last beta before rc our target for both tickets becomes 5.2. I think gap_stamp should go in DOT_SAGE, GAP_DIR seems to be reserved for some gap workspace and it may unwise to put it in there.

comment:19 Changed 7 years ago by ohanar

Actually GAP_DIR/workspace-*/ are the gap workspaces, so it should be fine to place in GAP_DIR.

comment:20 Changed 7 years ago by fbissey

You are right, I missed a line or two in the original file. Looks good to me.

comment:21 follow-up: Changed 7 years ago by fbissey

Question, why do we sometimes get the SAGE_* variables from os.environ and sometimes from sage.misc.misc? Should we try to be uniform and do it one way only?

comment:22 in reply to: ↑ 21 Changed 7 years ago by ohanar

Replying to fbissey:

Question, why do we sometimes get the SAGE_* variables from os.environ and sometimes from sage.misc.misc? Should we try to be uniform and do it one way only?

I would have it imported from sage.misc.misc (well actually I would move it to sage.misc.env, but that currently doesn't exist). I think that I only left one instance of os.environ between SAGE_DATA and SAGE_EXTCODE with this patch (I haven't touched the other SAGE_* with this patch).

Last edited 7 years ago by ohanar (previous) (diff)

comment:23 follow-up: Changed 7 years ago by ohanar

FYI, looking at the code there might be an issue with using a GAP_STAMP shared by multiple copies of Sage (I'm not positive, I don't really know the code). For right now I'm using the SAGE_EXTCODE directory as the GAP_STAMP.

comment:24 in reply to: ↑ 23 ; follow-up: Changed 7 years ago by fbissey

Replying to ohanar:

FYI, looking at the code there might be an issue with using a GAP_STAMP shared by multiple copies of Sage (I'm not positive, I don't really know the code). For right now I'm using the SAGE_EXTCODE directory as the GAP_STAMP.

Putting it in SAGE_EXTCODE is about as bad as putting it in SAGE_LOCAL/bin as it was initially. If you are concerned about multiple sage session running in parallel that's would still be a problem wherever you put it. I am not sure how the file is used but we don't remove it or anything. The real question is: is it actively used by gap? If gap just check for it's presence then it is OK. If it is used then it may have to be unique for each sage session and be cleaned on exit.

comment:25 in reply to: ↑ 24 Changed 7 years ago by ohanar

Replying to fbissey:

Putting it in SAGE_EXTCODE is about as bad as putting it in SAGE_LOCAL/bin as it was initially.

Not really, since it removes yet another instance of sage trying to write to someplace it shouldn't be at runtime.

The purpose of GAP_STAMP seems to be a check on whether it needs to refresh the workspaces. Really, this will all become a moot point if/when libgap is finished.

comment:26 follow-up: Changed 7 years ago by fbissey

As far as I am concerned SAGE_EXTCODE is a place sage shouldn't write at runtime. On a system wide installation that would be a folder that a user is not supposed to own. If a sys-admin install sage under /usr/local/sage-xxx for example, it would resolve to /usr/local/sage-xxx/local/share/sage/ext and as a sys-admin I object that my users have to write something there. Their home folder, tmp and any scratch space set for them is where it should go.

That's my perspective.

comment:27 in reply to: ↑ 26 Changed 7 years ago by ohanar

Replying to fbissey:

As far as I am concerned SAGE_EXTCODE is a place sage shouldn't write at runtime.

I don't write to SAGE_EXTCODE, I just use its creation for the timestamp that gap wants. It used to be that when you first started sage, it would touch SAGE_LOCAL/bin/gap_stamp and then use the timestamp of that.

I agree, nothing should try to write to SAGE_LOCAL or its subdirectories in the long run.

comment:28 Changed 7 years ago by fbissey

OK. If we just want a GAP_STAMP file for everyone and in SAGE_EXTCODE it should really be created by the extcode spkg. Is it a file we really need?

comment:29 Changed 7 years ago by ohanar

@fbissey

Do you think you are capable of finishing your review of this? (No pressure, just want to know if you can, so that I can find someone else to review it if you aren't able to.)

Last edited 7 years ago by ohanar (previous) (diff)

comment:30 Changed 7 years ago by fbissey

Yes, I will hopefully tomorrow. I was a bit down today - but this is important and we should move this.

comment:31 Changed 7 years ago by jdemeyer

Why local/share/sage/data and not local/share/sage or even local/share?

comment:32 Changed 7 years ago by jdemeyer

I would argue for local/share because the stuff in SAGE_DATA (apart from extcode) isn't part of Sage. It's part of external packages/databases. So I think it's wrong to use a sage subdirectory.

Installing databases in local/share/sage/data is like installing all binaries in local/bin/sage/binaries and we don't do that either.

For extcode, local/share/sage makes sense.

comment:33 Changed 7 years ago by jdemeyer

As far as I can tell, this ticket will break upgrading because an upgraded Sage will still have the SAGE_ROOT/data directory. This could be simply ignored and therefore harmless, but then it needs to be kept inside .hgignore. Alternatively, you could move SAGE_ROOT/data or even delete it.

Also, the database packages should depend on SAGE_ROOT_REPO, otherwise they won't see the new sage-env when upgrading.

comment:34 Changed 7 years ago by fbissey

I can agree with the SAGE_DATA location argument of Jeroen. The only argument against is that for most of them sage is the only application I can think of that make use of it. The splitting of the data is purely a convenience because it ease maintenance. In that case local/share/sage would be most appropriate.

comment:35 Changed 7 years ago by burcin

  • Cc burcin added

comment:36 Changed 7 years ago by ohanar

Sure, my main thing is just removing SAGE_ROOT/data and moving the databases that were there to some place in SAGE_LOCAL.

comment:37 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  • Summary changed from Move SAGE_DATA to SAGE_LOCAL/share/sage/data to Move SAGE_DATA to SAGE_LOCAL/share

Changed 7 years ago by ohanar

for review purposes

Changed 7 years ago by ohanar

for review purpose

Changed 7 years ago by ohanar

for review purposes

Changed 7 years ago by ohanar

for review purposes

comment:38 Changed 7 years ago by ohanar

  • Status changed from needs_work to needs_review

Ok, moved everything to SAGE_LOCAL/share (added SAGE_SHARE for convenience), and removed the SAGE_DATA environment variable.

I also added a bit to SAGE_ROOT/spkg/install to handle upgrading.

comment:39 Changed 7 years ago by jdemeyer

Some optional/experimental packages use SAGE_DATA:

  1. cunningham_tables-1.0
  2. p_group_cohomology-2.1.2
  3. database_stein_watkins_mini.p0
  4. sage-mode-0.7
  5. database_cremona_ellcurve-20120827
  6. database_sloane_oeis-2005-12
  7. database_jones_numfield-v4
  8. soya-0.11.2.p0

Some optional packages use `$SAGE_ROOT/data':

  1. database_odlyzko_zeta
  2. database_symbolic_data-20070206
  3. database_kohel-20060803

For these reasons, I would keep SAGE_DATA as synonym of SAGE_SHARE and also make a symlink

$SAGE_ROOT/data -> local/share
Last edited 7 years ago by jdemeyer (previous) (diff)

comment:40 Changed 7 years ago by fbissey

Keeping SAGE_DATA for a little while as a symlink definitely sound like a good idea. I don't remember explain_picklejar to be removed in trac13123_library.patch before nor do I understand the rational for the removal. Care to shade some light on that?

comment:41 Changed 7 years ago by jdemeyer

In the extcode patch, this comment is clearly wrong:

cp -pR * "$SAGE_ROOT/devel/ext-main/" # Creates new sagenb-main dir

comment:42 Changed 7 years ago by jdemeyer

Why change the logic in sage/databases/cremona.py?

comment:43 Changed 7 years ago by jdemeyer

In extcode spkg-install, why do

if [ -d .hg ]

You know that .hg exists (if not, that would be a bug).

comment:44 Changed 7 years ago by jdemeyer

spkg/install: indentation should be 4 spaces.

comment:45 Changed 7 years ago by ohanar

  • Status changed from needs_review to needs_work

I changed the logic in sage.databases.cremona since I wrote it, and it when replacing everything, it annoyed me that it wasn't factored appropriately.

I'll see about fixing the extcode spkg-install (most of the code there comes from a recent version of the sagenb spkg-install) andspkg/install, as well as adding a symlink for SAGE_DATA.

Changed 7 years ago by ohanar

apply to extcode repo

Changed 7 years ago by ohanar

apply to scripts repo

comment:46 Changed 7 years ago by ohanar

  • Status changed from needs_work to needs_review

I also fixed the commit message for the patches for the sage repos (I had already fixed the commit messages for the spkgs).

comment:47 Changed 7 years ago by jdemeyer

I would really prefer to separate the refactoring of the Cremona database code from this ticket (and then have either this ticket depend on that or the other way around).

comment:48 Changed 7 years ago by jdemeyer

  • Reviewers changed from François Bissey to François Bissey, Jeroen Demeyer

comment:49 Changed 7 years ago by jdemeyer

sage/interfaces/gap.py: why change hash(SAGE_ROOT) to hash(SAGE_LOCAL)? I don't understand why you made that change, nor the implications of that change.

comment:50 Changed 7 years ago by jdemeyer

  • Description modified (diff)

Based on purely reading the patches: positive review modulo my reviewer patches and the Cremona and GAP changes that I mentioned.

In any case, this still needs massive amounts of testing.

comment:51 Changed 7 years ago by fbissey

Sorry but I still don't understand the changes in sage/misc/explain_pickle.py and how they are related to the rest of the ticket. It may just be due to a lack of understanding on my part on the working of the pickle jar but the change seems unrelated to the rest of the ticket. Could someone offer me an explanation?

Last edited 7 years ago by fbissey (previous) (diff)

comment:52 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Changed 7 years ago by jdemeyer

comment:53 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:54 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This should be fixed:

sage -t --optional --long "devel/sage/sage/databases/symbolic_data.py"
**********************************************************************
File "/release/sage-5.4.beta2-try13123/devel/sage/sage/databases/symbolic_data.py", line 37:
    sage: sd = SymbolicData(); sd # optional - database_symbolic_data
Exception raised:
    Traceback (most recent call last):
      File "/release/sage-5.4.beta2-try13123/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/release/sage-5.4.beta2-try13123/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/release/sage-5.4.beta2-try13123/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_0[2]>", line 1, in <module>
        sd = SymbolicData(); sd # optional - database_symbolic_data###line 37:
    sage: sd = SymbolicData(); sd # optional - database_symbolic_data
      File "/release/sage-5.4.beta2-try13123/local/lib/python/site-packages/sage/databases/symbolic_data.py", line 78, in __init__
        path=os.path.join(sage.misc.misc.SAGE_SHARE, 'symbolic_data')
    NameError: global name 'sage' is not defined
**********************************************************************

comment:55 Changed 7 years ago by jdemeyer

Also, I can't get the sloane_database optional doctests to work in sage/databases/sloane.py, but I don't think it's because of this ticket.

Changed 7 years ago by ohanar

apply to sage library

comment:56 Changed 7 years ago by ohanar

  • Status changed from needs_work to needs_review

@fbissey

Because the commented out code references SAGE_DATA/extcode and hasn't been used in a long long time (see #6233).

Ok, updated. Changes:

  • Removed refactoring of sage.databases.cremona
  • Reverted change of SAGE_ROOT -> SAGE_LOCAL in sage.interfaces.gap (better done in #13432)
  • fixed sage.databases.symbolic_data
Last edited 7 years ago by ohanar (previous) (diff)

comment:57 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

sage --bdist doesn't copy devel/ext, leading to non-working binary builds.

comment:58 Changed 7 years ago by jdemeyer

  • Dependencies set to #13457

comment:59 Changed 7 years ago by jdemeyer

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

comment:60 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:61 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Changed 7 years ago by jdemeyer

comment:62 Changed 7 years ago by jdemeyer

Tested building from scratch, sdist, bdist, works fine. Positive review to everything not authored by me.

I agree with the deleting of the commented-out code in sage/misc/explain_pickle.py. If people really want to do something with that code, it's still in the Mercurial history.

My reviewer patches still need review.

comment:63 Changed 7 years ago by ohanar

  • Authors changed from R. Andrew Ohana to R. Andrew Ohana, Jeroen Demeyer

comment:64 Changed 7 years ago by ohanar

  • Status changed from needs_review to needs_work

Mercurial should no longer be marked as a dependency for the extcode spkg (it is no longer used in the spkg-install).

Changed 7 years ago by jdemeyer

comment:65 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:66 Changed 7 years ago by fbissey

  • Status changed from needs_review to positive_review

Looks all clear to me. I didn't really notice that the explain_pickle code was commented out last week, probably because of other stuff going on locally. But still happy that this is related to the rest of the ticket and not a freebie done at the same time.

As far as I can see it is ready to go.

comment:67 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.4.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed

Changed 7 years ago by jdemeyer

apply to root repo

comment:68 Changed 7 years ago by jdemeyer

Rebased trac13123_root.patch because of fuzz.

comment:69 Changed 7 years ago by jdemeyer

Why was GAP_STAMP changed from

GAP_STAMP = '%s/local/bin/gap_stamp'%SAGE_ROOT

to

GAP_STAMP = SAGE_EXTCODE

This seems a mistake and might be the cause of #13963.

Note: See TracTickets for help on using tickets.