add SAGE_SPKG_INST environment variable #14226

ohanar · 2013-03-05T07:34:06Z

There are a fair number of places that does stuff with the directory SAGE_ROOT/spkg/installed, so let's add an environment variable for this directory. This will help ease the transition to git.

Installation Instructions:

apply attachment: trac14226_library.patch to the sage library
apply attachment: trac14226_root.patch and attachment: 14226_root_review.patch to the root repository
apply attachment: trac14226_scripts.patch to the scripts repository

Depends on #13432
Depends on #14263

Component: misc

Keywords: installed spkgs directory

Author: R. Andrew Ohana

Reviewer: Robert Bradshaw, Leif Leonhardy, Jeroen Demeyer

Merged: sage-5.9.beta4

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

The text was updated successfully, but these errors were encountered:

ohanar · 2013-03-05T07:53:52Z

Dependencies: #13432

ohanar · 2013-03-05T07:53:52Z

Author: R. Andrew Ohana

ohanar · 2013-03-05T08:25:05Z

comment:3

breaks stuff... working on a fix...

ohanar · 2013-03-05T08:28:30Z

comment:4

Ok, found the missing link, should be good now.

nexttime · 2013-03-05T12:43:49Z

comment:6

Hmmm, I guess its name is pretty likely to confuse new users...

Note that $(INST) in the Makefile now contains an absolute path... (Similar for some scripts; although that doesn't break anything yet.)

tkluck · 2013-03-05T19:34:35Z

comment:7

The patch looks fine as a quick solution.

In general though, I think that this directory shouldn't be needed from different places at all. There should be two functions (or shell scripts, or ./sage options) like set_installed(name, bool) and is_installed(name) that know about how installation status is tracked, and only they are allowed to know about this directory. Having INST in the environment will allow/encourage people to check installation status themselves, and I think that should be prevented.

ohanar · 2013-03-10T03:24:43Z

comment:8

Replying to @tkluck:

In general though, I think that this directory shouldn't be needed from different places at all. There should be two functions (or shell scripts, or ./sage options) like set_installed(name, bool) and is_installed(name) that know about how installation status is tracked, and only they are allowed to know about this directory. Having INST in the environment will allow/encourage people to check installation status themselves, and I think that should be prevented.

Maybe, but at the same time it makes it a lot easier to find offending cases if we do move to a pair of functions like you propose, so at the very least this would be a step in the right direction.

robertwb · 2013-03-10T08:14:29Z

comment:9

Eventually we shouldn't be tracking this information ourselves at all (letting a package manager do it for us) so I'm fine with this "quick" solution. Users shouldn't have to use it, so the name being obscure isn't a big deal (and it's easy to discover reading the code).

nexttime · 2013-03-10T11:58:19Z

comment:10

$SAGE_INST needs to be quoted on line 339 of $SAGE_ROOT/spkg/install.

nexttime · 2013-03-10T12:02:12Z

comment:11

Replying to @nexttime:

$SAGE_INST needs to be quoted on line 339 of $SAGE_ROOT/spkg/install.

Same for deps, although I really don't like an absolute path there.

nexttime · 2013-03-10T12:05:29Z

comment:12

And although this variable is (or should be) only used internally, we should document it (with a warning that it doesn't refer to the location of a Sage installation, nor should be set/modified by the user).

2ct

nexttime · 2013-03-10T12:30:07Z

Reviewer: Robert Bradshaw, Leif Leonhardy

nexttime · 2013-03-10T12:30:07Z

Work Issues: Fix files in local/bin/ (scripts repo) as well. Probably document the new variable.

nexttime · 2013-03-10T12:30:07Z

comment:13

There are also instances in the Sage scripts repository ($SAGE_ROOT/local/bin/) which have to get fixed, e.g. in
sage-update:

    installed = set(os.listdir(os.path.join(SPKG_ROOT, "installed")))

Same in sage-list-packages:

try:
     installed = set(os.listdir(os.path.join(SPKG_ROOT, 'installed')))
except OSError:
     installed = set([])

There are presumably even more, but simple grepping yields false positives as well; I've found:

local/bin/sage-bdist:   cp $CP_OPT installed "$TMP/$PKGDIR/"

local/bin/sage-list-packages:     installed = set(os.listdir(os.path.join(SPKG_ROOT, 'installed')))

local/bin/sage-make_devel_packages:SPKG_INST="$SAGE_ROOT"/spkg/installed/

local/bin/sage-update:    installed = set(os.listdir(os.path.join(SPKG_ROOT, "installed")))

Looking at those, isn't SPKG_INST (or SAGE_SPKG_INST) a better name? ;-)

nexttime · 2013-03-17T09:33:58Z

comment:18

Replying to @ohanar:

Quoting the variable in deps brakes lots of stuff.

Apply sage-make_relative to it? XD

nexttime · 2013-03-17T09:37:23Z

comment:19

Seriously, if you really want an absolute path there, create a symbolic link from $(INST) to $SAGE_INST before running make?

Btw., what did it break?

nexttime · 2013-03-17T09:41:58Z

comment:20

Replying to @nexttime:

Seriously, if you really want an absolute path there, create a symbolic link from $(INST) to $SAGE_INST before running make?

(Where $(INST) is still relative, but its target is absolute.)

ohanar · 2013-03-17T09:47:40Z

comment:21

I don't mind if it is relative, but I don't want to be fixed to the location spkg/installed. The problem with a relative path is that then there are issues if using $SAGE_SPKG_INST from some other directory (which can and does happen) and sage-make_relative isn't available upon the first usage of deps. Making a symbolic link would work in the case that $(INST) != $SAGE_SPKG_INST.

nexttime · 2013-03-17T10:03:12Z

comment:22

Or, how does make INST="${SAGE_SPKG_INST:-installed}" behave? (I think this would be better.)

Replying to @ohanar:

I don't mind if it is relative, but I don't want to be fixed to the location spkg/installed.

I meant just in the Makefile. ($SAGE_SPKG_INST could still be absolute.)

The problem with a relative path is that then there are issues if using $SAGE_SPKG_INST from some other directory (which can and does happen)

See above.

sage-make_relative isn't available upon the first usage of deps.

I was just kidding.

Making a symbolic link would work in the case that $(INST) != $SAGE_SPKG_INST.

Well, just change INST = installed to something else (INST = this_is_always_the_relative_path_to_a_symlink), and let it point to either installed or $SAGE_SPKG_INST (where the latter is absolute). Not sure about performance issues.

ohanar · 2013-03-17T10:46:53Z

comment:23

Replying to @nexttime:

Or, how does make INST="${SAGE_SPKG_INST:-installed}" behave? (I think this would be better.)

Yes, this resolves the issue -- which was that any entry of the form $(MAKE) $(INST)/$(SPKG) would error out complaining about a missing target.

ohanar · 2013-03-17T10:46:53Z

Changed work issues from Don't quote in deps to none

nexttime · 2013-03-17T10:50:01Z

comment:24

Replying to @nexttime:

Or, how does make INST="${SAGE_SPKG_INST:-installed}" behave? (I think this would be better.)

After deep thinking...

make INST="`echo ${SAGE_SPKG_INST:-installed} | sed 's/ /\\\\ /g'`"

(without changes to deps).

I think this should solve all issues.

nexttime · 2013-03-17T11:00:24Z

comment:25

I'd keep the "default" value of INST in deps.

Also, I think there's more than one instance of $MAKE in spkg/install, not sure whether also elsewhere.

ohanar · 2013-03-17T11:19:55Z

comment:26

ok, made your suggested changes.

Testing locally I found no difference when introducing whitespace in the directory name of SAGE_SPKG_INST, but I have a very recent version of gmake installed on my system.

nexttime · 2013-03-17T11:42:32Z

comment:27

Replying to @ohanar:

Testing locally I found no difference when introducing whitespace in the directory name of SAGE_SPKG_INST, but I have a very recent version of gmake installed on my system.

Should^TM also work with

GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.

jdemeyer · 2013-03-26T21:19:35Z

Changed dependencies from #13432 to #13432, #14263

jdemeyer · 2013-03-26T21:20:05Z

apply to root repository

jdemeyer · 2013-03-26T21:20:20Z

comment:29

Attachment: trac14226_root.patch.gz

Rebased to #14263.

jdemeyer · 2013-03-26T22:29:14Z

Changed reviewer from Robert Bradshaw, Leif Leonhardy to Robert Bradshaw, Leif Leonhardy, Jeroen Demeyer

jdemeyer · 2013-03-26T22:29:14Z

comment:30

Attachment: 14226_root_review.patch.gz

jdemeyer · 2013-03-28T20:39:46Z

comment:31

Positive review given in person by R. Andrew Ohana.

jdemeyer · 2013-03-29T01:17:41Z

Merged: sage-5.9.beta3

jdemeyer · 2013-04-03T08:53:49Z

Changed merged from sage-5.9.beta3 to sage-5.9.beta4

ohanar added this to the sage-5.8 milestone Mar 5, 2013

ohanar added p: major / 3 labels Mar 5, 2013

ohanar assigned jasongrout Mar 5, 2013

This comment has been minimized.

Sign in to view

ohanar added the s: needs review label Mar 5, 2013

ohanar added s: needs work and removed s: needs review labels Mar 5, 2013

ohanar added s: needs review and removed s: needs work labels Mar 5, 2013

jdemeyer modified the milestones: sage-5.8, sage-5.9 Mar 5, 2013

robertwb added s: positive review and removed s: needs review labels Mar 10, 2013

nexttime mannequin added s: needs work and removed s: positive review labels Mar 10, 2013

ohanar added the s: needs work label Mar 17, 2013

ohanar added s: needs review and removed s: needs work labels Mar 17, 2013

This comment has been minimized.

Sign in to view

jdemeyer added s: positive review and removed s: needs review labels Mar 28, 2013

jdemeyer removed the s: positive review label Mar 29, 2013

jdemeyer closed this as completed Mar 29, 2013

ohanar mentioned this issue Aug 31, 2017

add sage/env.py and fix many inappropriate references to SAGE_ROOT #13432

Closed

This was referenced May 12, 2013

Move spkg logs to logs/pkgs #14377

Closed

Upgrade GCC to 4.7.2 #14378

Closed

add SAGE_SPKG_INST environment variable #14226

add SAGE_SPKG_INST environment variable #14226

Comments

ohanar commented Mar 5, 2013

This comment has been minimized.

ohanar commented Mar 5, 2013

ohanar commented Mar 5, 2013

ohanar commented Mar 5, 2013

ohanar commented Mar 5, 2013

nexttime mannequin commented Mar 5, 2013

tkluck commented Mar 5, 2013

ohanar commented Mar 10, 2013

robertwb commented Mar 10, 2013

nexttime mannequin commented Mar 10, 2013

nexttime mannequin commented Mar 10, 2013

nexttime mannequin commented Mar 10, 2013

nexttime mannequin commented Mar 10, 2013

nexttime mannequin commented Mar 10, 2013

nexttime mannequin commented Mar 10, 2013

nexttime mannequin commented Mar 17, 2013

nexttime mannequin commented Mar 17, 2013

nexttime mannequin commented Mar 17, 2013

ohanar commented Mar 17, 2013

nexttime mannequin commented Mar 17, 2013

ohanar commented Mar 17, 2013

ohanar commented Mar 17, 2013

nexttime mannequin commented Mar 17, 2013

nexttime mannequin commented Mar 17, 2013

ohanar commented Mar 17, 2013

nexttime mannequin commented Mar 17, 2013

jdemeyer commented Mar 26, 2013

jdemeyer commented Mar 26, 2013

jdemeyer commented Mar 26, 2013

This comment has been minimized.

jdemeyer commented Mar 26, 2013

jdemeyer commented Mar 26, 2013

jdemeyer commented Mar 28, 2013

jdemeyer commented Mar 29, 2013

jdemeyer commented Apr 3, 2013