Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move SAGE_DATA to SAGE_LOCAL/share #13123

Closed
ohanar opened this issue Jun 16, 2012 · 78 comments
Closed

Move SAGE_DATA to SAGE_LOCAL/share #13123

ohanar opened this issue Jun 16, 2012 · 78 comments

Comments

@ohanar
Copy link
Member

ohanar commented Jun 16, 2012

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:

Depends on #13457

CC: @kini @jdemeyer @robertwb @burcin

Component: relocation

Author: R. Andrew Ohana, Jeroen Demeyer

Reviewer: François Bissey, Jeroen Demeyer

Merged: sage-5.4.beta2

Issue created by migration from https://trac.sagemath.org/ticket/13123

@ohanar ohanar added this to the sage-5.4 milestone Jun 16, 2012
@ohanar
Copy link
Member Author

ohanar commented Jun 18, 2012

Dependencies: #12205

@ohanar
Copy link
Member Author

ohanar commented Jun 18, 2012

Author: R. Andrew Ohana

@ohanar

This comment has been minimized.

@ohanar

This comment has been minimized.

@kini

This comment has been minimized.

@ohanar
Copy link
Member Author

ohanar commented Jun 19, 2012

Changed dependencies from #12205 to none

@ohanar

This comment has been minimized.

@ohanar
Copy link
Member Author

ohanar commented Jun 19, 2012

comment:5

Ok, everything should work now.

@ohanar

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Jun 19, 2012

comment:7

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.

@kiwifb
Copy link
Member

kiwifb commented Jun 19, 2012

Reviewer: François Bissey

@jdemeyer
Copy link

comment:8

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

@jdemeyer
Copy link

comment:9

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.

@jdemeyer
Copy link

comment:10

Why don't you

mkdir -p "$SAGE_DATA"

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

@jdemeyer
Copy link

comment:11

Replying to @kiwifb:

  • 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.

@ohanar
Copy link
Member Author

ohanar commented Jun 19, 2012

comment:12

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.

@ohanar
Copy link
Member Author

ohanar commented Jun 19, 2012

comment:13

Replying to @kiwifb:

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.

@ohanar

This comment has been minimized.

@ohanar
Copy link
Member Author

ohanar commented Jun 19, 2012

comment:15

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.

@kiwifb
Copy link
Member

kiwifb commented Jun 19, 2012

comment:16

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?

@ohanar
Copy link
Member Author

ohanar commented Jun 19, 2012

comment:17

Replying to @kiwifb:

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.)

@kiwifb
Copy link
Member

kiwifb commented Jun 19, 2012

comment:18

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.

@ohanar
Copy link
Member Author

ohanar commented Jun 19, 2012

comment:19

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

@kiwifb
Copy link
Member

kiwifb commented Jun 19, 2012

comment:20

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

@kiwifb
Copy link
Member

kiwifb commented Jun 28, 2012

comment:21

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?

@ohanar
Copy link
Member Author

ohanar commented Jun 29, 2012

comment:22

Replying to @kiwifb:

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).

@jdemeyer
Copy link

Attachment: 13123_bdist.patch.gz

@jdemeyer
Copy link

comment:62

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.

@ohanar
Copy link
Member Author

ohanar commented Sep 16, 2012

Changed author from R. Andrew Ohana to R. Andrew Ohana, Jeroen Demeyer

@ohanar
Copy link
Member Author

ohanar commented Sep 16, 2012

comment:64

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

@jdemeyer
Copy link

Attachment: 13123_root_review.patch.gz

@kiwifb
Copy link
Member

kiwifb commented Sep 17, 2012

comment:66

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.

@jdemeyer
Copy link

Merged: sage-5.4.beta2

@jdemeyer
Copy link

apply to root repo

@jdemeyer
Copy link

comment:68

Attachment: trac13123_root.patch.gz

Rebased attachment: trac13123_root.patch because of fuzz.

@jdemeyer
Copy link

comment:69

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants