add SAGE_SPKG_INST environment variable

[ohanar]

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 trac14226_library.patch​ to the sage library
• apply trac14226_root.patch​ and 14226_root_review.patch​ to the root repository
• apply trac14226_scripts.patch​ to the scripts repository

[ohanar]

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

[ohanar]

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

[leif]

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]

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]

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]

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

[leif]

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

[leif]

Replying to leif:

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

[leif]

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

[leif]

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? ;-)

[ohanar]

So my only objection to SPKG_INST is that it follows the convention of SPKG_ROOT which has been replaced with SAGE_PACKAGES, so it seems like a slight regression.

On the other hand, SPKG_INST makes sense in the context of trying to move to a real package manager, and that this directory only records the installations of spkgs (as opposed to packages installed with other methods). From this perspective, I agree that SPKG_INST would be a better name (enough to outweigh my objection).

As for documentation, from what I can tell there is no documentation on any development related variables, so I've made a new ticket for the task. (#14262)

[ohanar]

apply to scripts repository

[ohanar]

apply to sage library

[ohanar]

Quoting the variable in deps brakes lots of stuff.

[leif]

Replying to ohanar:

Quoting the variable in deps brakes lots of stuff.

Apply sage-make_relative to it? XD

[leif]

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?

[leif]

Replying to leif:

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]

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.

[leif]

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]

Replying to leif:

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.

[leif]

Replying to leif:

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.

[leif]

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]

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.

[leif]

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]

apply to root repository

[jdemeyer]

Rebased to #14263.

[jdemeyer]

Positive review given in person by R. Andrew Ohana.