#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 8 years ago by
- Dependencies set to #13432
- Description modified (diff)
comment:2 Changed 8 years ago by
- Status changed from new to needs_review
comment:3 Changed 8 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 8 years ago by
- Status changed from needs_work to needs_review
Ok, found the missing link, should be good now.
comment:5 Changed 8 years ago by
- Milestone changed from sage-5.8 to sage-5.9
comment:6 Changed 8 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 8 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 in reply to: ↑ 7 Changed 8 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 8 years ago by
- Status changed from needs_review to 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 8 years ago by
$SAGE_INST
needs to be quoted on line 339 of $SAGE_ROOT/spkg/install
.
comment:11 in reply to: ↑ 10 Changed 8 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 8 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 8 years ago by
- Reviewers set to Robert Bradshaw, Leif Leonhardy
- Status changed from positive_review to needs_work
- Work issues set to 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 8 years ago by
- Keywords installed spkgs directory added
- Work issues changed from Fix files in `local/bin/` (scripts repo) as well. Probably document the new variable. to 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 8 years ago by
- Work issues changed from Fix files in `local/bin/` (scripts repo) as well. Quote two instances in `deps` and `spkg/install`. Probably document the new variable. to 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 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Summary changed from add SAGE_INST environment variable to 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`. deleted
comment:17 follow-up: ↓ 18 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to Don't quote in deps
Quoting the variable in deps brakes lots of stuff.
comment:18 in reply to: ↑ 17 Changed 8 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 8 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 in reply to: ↑ 19 Changed 8 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 8 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 in reply to: ↑ 21 ; follow-up: ↓ 23 Changed 8 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 in reply to: ↑ 22 Changed 8 years ago by
- Status changed from needs_work to needs_review
- Work issues Don't quote in deps deleted
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 8 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 8 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 8 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 in reply to: ↑ 26 Changed 8 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 8 years ago by
- Dependencies changed from #13432 to #13432, #14263
comment:29 Changed 8 years ago by
Rebased to #14263.
Changed 8 years ago by
comment:30 Changed 8 years ago by
- Description modified (diff)
- Reviewers changed from Robert Bradshaw, Leif Leonhardy to Robert Bradshaw, Leif Leonhardy, Jeroen Demeyer
comment:31 Changed 8 years ago by
- Status changed from needs_review to positive_review
Positive review given in person by R. Andrew Ohana.
comment:32 Changed 8 years ago by
- Merged in set to sage-5.9.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:33 Changed 8 years ago by
- Merged in changed from sage-5.9.beta3 to sage-5.9.beta4
breaks stuff... working on a fix...