Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#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 jdemeyer)

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:

Attachments (4)

trac14226_scripts.patch (3.8 KB) - added by ohanar 8 years ago.
apply to scripts repository
trac14226_library.patch (1.3 KB) - added by ohanar 8 years ago.
apply to sage library
trac14226_root.patch (5.7 KB) - added by jdemeyer 8 years ago.
apply to root repository
14226_root_review.patch (1.9 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 8 years ago by ohanar

  • Authors set to R. Andrew Ohana
  • Dependencies set to #13432
  • Description modified (diff)

comment:2 Changed 8 years ago by ohanar

  • Status changed from new to needs_review

comment:3 Changed 8 years ago by ohanar

  • Status changed from needs_review to needs_work

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

comment:4 Changed 8 years ago by ohanar

  • Status changed from needs_work to needs_review

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

comment:5 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.8 to sage-5.9

comment:6 Changed 8 years ago by 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.)

comment:7 follow-up: Changed 8 years ago by 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.

comment:8 in reply to: ↑ 7 Changed 8 years ago by 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.

comment:9 Changed 8 years ago by robertwb

  • 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: Changed 8 years ago by leif

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

comment:12 Changed 8 years ago by 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

comment:13 Changed 8 years ago by leif

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

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

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

Changed 8 years ago by ohanar

apply to scripts repository

Changed 8 years ago by ohanar

apply to sage library

comment:16 Changed 8 years ago by ohanar

  • 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: Changed 8 years ago by ohanar

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

Replying to ohanar:

Quoting the variable in deps brakes lots of stuff.

Apply sage-make_relative to it? XD

comment:19 follow-up: Changed 8 years ago by 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?

comment:20 in reply to: ↑ 19 Changed 8 years ago by 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.)

comment:21 follow-up: Changed 8 years ago by 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.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 8 years ago by 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.

comment:23 in reply to: ↑ 22 Changed 8 years ago by ohanar

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

Last edited 8 years ago by leif (previous) (diff)

comment:25 Changed 8 years ago by 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.

comment:26 follow-up: Changed 8 years ago by 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.

comment:27 in reply to: ↑ 26 Changed 8 years ago by 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.

ShouldTM also work with

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

comment:28 Changed 8 years ago by jdemeyer

  • Dependencies changed from #13432 to #13432, #14263

Changed 8 years ago by jdemeyer

apply to root repository

comment:29 Changed 8 years ago by jdemeyer

Rebased to #14263.

Changed 8 years ago by jdemeyer

comment:30 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers changed from Robert Bradshaw, Leif Leonhardy to Robert Bradshaw, Leif Leonhardy, Jeroen Demeyer

comment:31 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Positive review given in person by R. Andrew Ohana.

comment:32 Changed 8 years ago by jdemeyer

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

  • Merged in changed from sage-5.9.beta3 to sage-5.9.beta4
Note: See TracTickets for help on using tickets.