#14226 closed enhancement (fixed)
add SAGE_SPKG_INST environment variable
Reported by: | ohanar | Owned by: | jason |
---|---|---|---|
Priority: | major | Milestone: | sage-5.9 |
Component: | misc | Keywords: | installed spkgs directory |
Cc: | Merged in: | sage-5.9.beta4 | |
Authors: | R. Andrew Ohana | Reviewers: | Robert Bradshaw, Leif Leonhardy, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #13432, #14263 | Stopgaps: |
Description (last modified by )
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
Attachments (4)
Change History (37)
comment:1 Changed 10 years ago by
Authors: | → R. Andrew Ohana |
---|---|
Dependencies: | → #13432 |
Description: | modified (diff) |
comment:2 Changed 10 years ago by
Status: | new → needs_review |
---|
comment:3 Changed 10 years ago by
Status: | needs_review → needs_work |
---|
comment:4 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
Ok, found the missing link, should be good now.
comment:5 Changed 10 years ago by
Milestone: | sage-5.8 → sage-5.9 |
---|
comment:6 Changed 10 years ago by
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.)
comment:7 follow-up: 8 Changed 10 years ago by
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.
comment:8 Changed 10 years ago by
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.
comment:9 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
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).
comment:10 follow-up: 11 Changed 10 years ago by
$SAGE_INST
needs to be quoted on line 339 of $SAGE_ROOT/spkg/install
.
comment:11 Changed 10 years ago by
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.
comment:12 Changed 10 years ago by
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
comment:13 Changed 10 years ago by
Reviewers: | → Robert Bradshaw, Leif Leonhardy |
---|---|
Status: | positive_review → needs_work |
Work issues: | → Fix files in `local/bin/` (scripts repo) as well. Probably document the new variable. |
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? ;-)
comment:14 Changed 10 years ago by
Keywords: | installed spkgs directory added |
---|---|
Work issues: | Fix files in `local/bin/` (scripts repo) as well. Probably document the new variable. → Fix files in `local/bin/` (scripts repo) as well. Quote two instances in `deps` and `spkg/install`. Probably document the new variable. |
comment:15 Changed 10 years ago by
Work issues: | Fix files in `local/bin/` (scripts repo) as well. Quote two instances in `deps` and `spkg/install`. Probably document the new variable. → Fix files in `local/bin/` (scripts repo) as well. Quote two instances in `deps` and `spkg/install`. |
---|
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)
comment:16 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
Summary: | add SAGE_INST environment variable → add SAGE_SPKG_INST environment variable |
Work issues: | Fix files in `local/bin/` (scripts repo) as well. Quote two instances in `deps` and `spkg/install`. |
comment:17 follow-up: 18 Changed 10 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → Don't quote in deps |
Quoting the variable in deps brakes lots of stuff.
comment:18 Changed 10 years ago by
Replying to ohanar:
Quoting the variable in deps brakes lots of stuff.
Apply sage-make_relative
to it? XD
comment:19 follow-up: 20 Changed 10 years ago by
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?
comment:20 Changed 10 years ago by
Replying to leif:
Seriously, if you really want an absolute path there, create a symbolic link from
$(INST)
to$SAGE_INST
before runningmake
?
(Where $(INST)
is still relative, but its target is absolute.)
comment:21 follow-up: 22 Changed 10 years ago by
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
.
comment:22 follow-up: 23 Changed 10 years ago by
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 ofdeps
.
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.
comment:23 Changed 10 years ago by
Status: | needs_work → needs_review |
---|---|
Work issues: | Don't quote in deps |
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.
comment:24 Changed 10 years ago by
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.
comment:25 Changed 10 years ago by
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.
comment:26 follow-up: 27 Changed 10 years ago by
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.
comment:27 Changed 10 years ago by
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.
ShouldTM also work with
GNU Make 3.81 Copyright (C) 2006 Free Software Foundation, Inc.
comment:28 Changed 10 years ago by
Dependencies: | #13432 → #13432, #14263 |
---|
Changed 10 years ago by
Attachment: | 14226_root_review.patch added |
---|
comment:30 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | Robert Bradshaw, Leif Leonhardy → Robert Bradshaw, Leif Leonhardy, Jeroen Demeyer |
comment:31 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
Positive review given in person by R. Andrew Ohana.
comment:32 Changed 10 years ago by
Merged in: | → sage-5.9.beta3 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:33 Changed 10 years ago by
Merged in: | sage-5.9.beta3 → sage-5.9.beta4 |
---|
breaks stuff... working on a fix...