#15105 closed enhancement (fixed)
hardwired paths in src/sage
Reported by: | felixs | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.5 |
Component: | build | Keywords: | environment, SAGE_LOCAL, hardwired, run time |
Cc: | jondo, fbissey, mkoeppe, dimpase, vbraun, kcrisman, mmarco, nbruin | Merged in: | |
Authors: | Felix Salfelder, Erik Bray, Matthias Koeppe | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 45c5dd0 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
Several run time paths in the python library are hardwired, in most cases to $SAGE_LOCAL
.
$SAGE_LOCAL
has no meaning outside of Sage (the distribution). So it's better to add them to sage-env.py and make them overridable.
For example in libs.singular.singular_init
, the singular shared object is looked for in $SAGE_LOCAL/lib
.
Change History (123)
comment:1 Changed 9 years ago by
- Branch set to u/felixs/runtime_paths
- Commit set to 2cffff84404e4179adc19860b29c5bae9ed80742
comment:2 Changed 9 years ago by
- Dependencies set to #15100
comment:3 Changed 9 years ago by
- Cc jondo added
comment:4 Changed 9 years ago by
- Commit changed from 2cffff84404e4179adc19860b29c5bae9ed80742 to c19a5f3e5c8e61bb3e6978c8646288f5094ed1b0
Branch pushed to git repo; I updated commit sha1. New commits:
[changeset:c19a5f3] | unhardwire sandpile |
[changeset:5746289] | unhardwire polymake path |
[changeset:8f8a246] | get sage-started.txt from SAGE_ETC |
[changeset:ee831c1] | unardwire sage-CSI call in csage |
[changeset:16bf607] | initialize SAGE_ETC (from environ) in sage-location before use |
comment:5 Changed 9 years ago by
- Commit changed from c19a5f3e5c8e61bb3e6978c8646288f5094ed1b0 to 591471ddb968765c4582ed0ccf7ce4e250443c3e
Branch pushed to git repo; I updated commit sha1. New commits:
[changeset:591471d] | python-sage: whitelist python2.7 multiprocessing in stackframes test |
comment:6 Changed 9 years ago by
- Status changed from new to needs_review
comment:7 Changed 9 years ago by
- Commit changed from 591471ddb968765c4582ed0ccf7ce4e250443c3e to 0b9cb04e432036e47e1fb735f2044b73e4f0ae58
Branch pushed to git repo; I updated commit sha1. New commits:
[changeset:0b9cb04] | doctests in src/sage/plot/misc.py |
[changeset:1ef62a1] | unhardwire cython.py completed |
[changeset:390e28b] | qepcard doctest fix: singular might well be in /usr/bin, not .../local/bin |
[changeset:0dcd721] | unhardwire ELLCURVES_DATA_DIR |
comment:8 follow-up: ↓ 9 Changed 8 years ago by
- Status changed from needs_review to needs_work
Needs to be rebased. Given that I totally disagree with #15100, preferably this ticket should not depend on that.
comment:9 in reply to: ↑ 8 Changed 8 years ago by
Replying to jdemeyer:
Needs to be rebased. Given that I totally disagree with #15100, preferably this ticket should not depend on that.
#15100 is not too important. particularly for standalone sagelib it doesn't matter. anyhow, the doctest checking the fist line of the ipython (or any other) executable needs to be removed.
comment:10 Changed 8 years ago by
- Cc fbissey added
comment:11 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:12 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:13 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:14 Changed 6 years ago by
- Cc mkoeppe added
comment:15 Changed 6 years ago by
This patch needs rebasing
comment:16 Changed 6 years ago by
- Commit changed from 0b9cb04e432036e47e1fb735f2044b73e4f0ae58 to 12f698526f1385923acc5c1b2b0c855ea908e3c1
comment:17 Changed 6 years ago by
- Commit changed from 12f698526f1385923acc5c1b2b0c855ea908e3c1 to 8149a85cae9ea41685a0954a8d5b275caef16807
Branch pushed to git repo; I updated commit sha1. New commits:
8149a85 | Merge 'develop' into runtime_paths
|
comment:18 Changed 6 years ago by
this is now rebased.
i tried to resolve the conflicts carefully, but i haven't compiled/tested anything. can't do that anytime soon (no time, no hardware).
hth
comment:19 follow-up: ↓ 21 Changed 6 years ago by
You still have stuff from old version of sage that are not there anymore. All the files where the diff starts with /dev/null
. Better get rid of them.
comment:20 Changed 6 years ago by
- Commit changed from 8149a85cae9ea41685a0954a8d5b275caef16807 to 4846a73aad7cf7361c69a6ae43628b8cae821b9f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4846a73 | merge develop into runtime_paths
|
comment:21 in reply to: ↑ 19 Changed 6 years ago by
Replying to fbissey:
Better get rid of them.
thanks. missed that. i took that opportunity to squash the three merges into one.
comment:22 Changed 6 years ago by
Instead of sage.misc.misc
, shouldn't it use sage.env
?
comment:23 Changed 6 years ago by
--- a/src/bin/sage-location +++ b/src/bin/sage-location @@ -6,6 +6,7 @@ import os, sys, re SAGE_ROOT = os.path.realpath(os.environ['SAGE_ROOT']) SAGE_LOCAL = os.environ['SAGE_LOCAL'] +SAGE_ETC = os.environ['SAGE_ETC'] location_file = os.path.join(SAGE_LOCAL, 'lib', 'sage-current-location.txt') force_file = os.path.join(SAGE_LOCAL, 'lib', 'sage-force-relocate.txt') @@ -215,8 +216,8 @@ def write_config_files(): path. """ # Currently only one file (for the experimental package qepcad): - # $SAGE_ROOT/local/default.qepcadrc - f = open(os.path.join(SAGE_LOCAL, 'default.qepcadrc'), 'w') + # $SAGE_ROOT/local/etc/default.qepcadrc + f = open(os.path.join(SAGE_ETC, 'default.qepcadrc'), 'w') text = \ """# THIS FILE IS AUTOMATICALLY GENERATED by sage-location -- DO NOT EDIT
I raised the issue of qepcadrc
before,but it has to be done in conjunction of fixing the spkg in question.
In sage-on-gentoo I just got rid of sage-started.txt
it is one of those appendages that don't really belong anywhere sensible. /var/lib/sage
?
I am not sure about the catalogue of DATA_DIR
and INCLUDE_DIR
(and in line with my previous post csage
is gone).
Did you know that lib{s,S}ingular is the only file that is dlopen-ed in sage. There should be a better way of getting to it.
Do not ever use SAGE_SRC
for something that's needed at runtime. I could let go of that for doctesting as you do, but that makes things harder to test in sage-on-gentoo.
Overall I am not overly impressed by the amount of stuff showed in env.py
. Some of that stuff may also conflict with #20382 which I very much would like to see merged.
comment:24 follow-ups: ↓ 25 ↓ 26 Changed 6 years ago by
I raised the issue of qepcadrc before,but it has to be done in conjunction of fixing the spkg in question.
sage_location (whatever it tries to do) should be moved out of sagelib. it's not needed there. i don't remember what qepcadrc was exactly about, but there was headache involved.
In sage-on-gentoo I just got rid of sage-started.txt
maybe sage should touch ~/.sage/started instead? this is often writable... does sage-started.txt implement anything useful at all?
Did you know that lib{s,S}ingular is the only file that is dlopen-ed in sage. There should be a better way of getting to it.
it's hard to get a broken implementation right. iirc theres has never been a technical reason for not just linking. so maybe... just link? (see my ancient comment in env.py).
Do not ever use SAGE_SRC for something that's needed at runtime. I could let go of that for doctesting as you do, but that makes things harder to test in sage-on-gentoo.
there is some sort of interactive source browser. how would that work without SAGE_SRC? i think i did this [1] (also installed the source) to fix it.
stuff showed in env.py
too many *DIR variables? true. there are better ways: each extension should have its own configuration (read: should be a standalone package depending on sagelib). i think, we are simply not there yet. cf. [2] for the intended (workaround) implementation.
please go ahead and finish that first. this ticket was never meant to interfere with #20382.
[1] https://github.com/felix-salfelder/sage/blob/autotools/src/sage/env.py.in#L103 [2] https://github.com/felix-salfelder/sage/blob/autotools/src/sage/env.py.in#L134
comment:25 in reply to: ↑ 24 Changed 6 years ago by
Replying to felixs:
stuff showed in env.py
too many *DIR variables? true. there are better ways: each extension should have its own configuration (read: should be a standalone package depending on sagelib). i think, we are simply not there yet. cf. [2] for the intended (workaround) implementation.
I agree with Felix on this.
We do need lots of these variables, because we no longer want to assume that everything lives in the same prefix hierarchy ("SAGE_LOCAL").
Whether these kind of configuration variables should go into env or into another centralized module, is another question.
comment:26 in reply to: ↑ 24 ; follow-up: ↓ 27 Changed 6 years ago by
Replying to felixs:
I raised the issue of qepcadrc before,but it has to be done in conjunction of fixing the spkg in question.
sage_location (whatever it tries to do) should be moved out of sagelib. it's not needed there. i don't remember what qepcadrc was exactly about, but there was headache involved.
In sage-on-gentoo I just got rid of sage-started.txt
maybe sage should touch ~/.sage/started instead? this is often writable... does sage-started.txt implement anything useful at all?
No, it was introduced as a check by Jeroen (when he was the release manager) to check that sage does starts. So it is produced at the end of the build process and checked and regenerated if missing when sage is started.
Did you know that lib{s,S}ingular is the only file that is dlopen-ed in sage. There should be a better way of getting to it.
it's hard to get a broken implementation right. iirc theres has never been a technical reason for not just linking. so maybe... just link? (see my ancient comment in env.py).
That a novel idea :) but even the singular4 ticket isn't going there.
Do not ever use SAGE_SRC for something that's needed at runtime. I could let go of that for doctesting as you do, but that makes things harder to test in sage-on-gentoo.
there is some sort of interactive source browser. how would that work without SAGE_SRC? i think i did this [1] (also installed the source) to fix it.
I don't really know what that does. Never used it. So in sage-on-gentoo SAGE_SRC
points to /usr/lib{,64}/python2.7/site-packages/sage
and I install the necessary pxd
and pyx
to run test. So that probably works. But my attitude on that is that the source shouldn't be considered available at runtime and of course you should be able to run tests before installing. The possibility of doing the last bit is shot down by #21221.
stuff showed in env.py
too many *DIR variables? true. there are better ways: each extension should have its own configuration (read: should be a standalone package depending on sagelib). i think, we are simply not there yet. cf. [2] for the intended (workaround) implementation.
please go ahead and finish that first. this ticket was never meant to interfere with #20382.
[1] https://github.com/felix-salfelder/sage/blob/autotools/src/sage/env.py.in#L103 [2] https://github.com/felix-salfelder/sage/blob/autotools/src/sage/env.py.in#L134
Yes I guess if you are thinking of a modular approach where components can be installed anywhere on the system, like hashdist or spack, you want to use variable like CPATH and LIBRARY_PATH, set when you load the components in the environment. Stuff under SAGE_LOCAL/share is still an issue in that case.
On a system like sage-on-gentoo where things still live under a single prefix (but is a more conventional one) keeping SAGE_LOCAL is not a big deal. Apart from etc
I just use it like it was --prefix=/usr
.
comment:27 in reply to: ↑ 26 Changed 6 years ago by
Replying to fbissey:
i appreciate your clear reply. but i'm out of time. maybe next week.
comment:28 Changed 6 years ago by
About sage-started.txt
: it has two purposes:
- Check that Sage starts.
- Initialize any files which should be initialized at build time (this includes running
sage-location
and generating.pyc
files). See #11926.
comment:29 Changed 6 years ago by
Regarding
# hmm, who is going to create sage_started if SAGE_ETC is readonly?
This isn't going to happen since this file should be written only by the build process. At that point, SAGE_ETC
should be writable.
So please remove this comment.
comment:30 Changed 6 years ago by
What is the point of this?
# Do nothing if the file already exists if os.path.isfile(started_file): return
make
expects the timestamp to be updated.
comment:31 Changed 6 years ago by
sage_started is now in #21510
comment:32 Changed 6 years ago by
OK, best to remove anything about sage_started from the present ticket's branch then.
comment:33 Changed 6 years ago by
sage_location now is #21511.
comment:34 Changed 6 years ago by
- Milestone changed from sage-6.4 to sage-7.5
comment:35 Changed 6 years ago by
See also #21591.
comment:36 Changed 6 years ago by
Definitely agree that none of Sage's external dependencies should be assumed to live under the same prefix. I think it's fine for the sake of using Sage-the-distribution to have defaults that assume everything is under $SAGE_LOCAL
, but all these paths should be easy for system integrator to change (it might nice to even have a simple config file read at build time).
comment:37 follow-up: ↓ 38 Changed 6 years ago by
I rebased this properly on top of develop, rather than trying to merge develop into it, if anyone would like to have a look. I think I got everything right but I might be missing one or two things. I'm not sure what to do with the new SAGE_MAXIMA_INIT
this adds.
comment:38 in reply to: ↑ 37 ; follow-up: ↓ 39 Changed 6 years ago by
Replying to embray:
I rebased this properly on top of develop
thanks. can you update the "Branch" field? (is it supposed to work like that?)
I'm not sure what to do with the new
SAGE_MAXIMA_INIT
this adds.
iirc, this variable was needed to point to some maxima fragment, which is in different places, depending on where sage is run from. obviously there must be a better solution (including one that does not use a file like this one).
comment:39 in reply to: ↑ 38 Changed 6 years ago by
Replying to felixs:
Replying to embray:
I rebased this properly on top of develop
thanks. can you update the "Branch" field? (is it supposed to work like that?)
The branch definitely needs to be changed unless Erik has the right to write to felixs
' branches ;) Cannot comment until I see the new branch.
comment:40 follow-up: ↓ 41 Changed 6 years ago by
I can change the branch to mine, but only if it's ok with felixs. I think I need to pull out the SAGE_MAXIMA_INIT
stuff which isn't ready, but otherwise I think it brings it most other changes from the old version of his branch that are still relevant.
comment:41 in reply to: ↑ 40 Changed 6 years ago by
comment:42 Changed 6 years ago by
- Branch changed from u/felixs/runtime_paths to u/embray/ticket-15105
- Commit changed from 4846a73aad7cf7361c69a6ae43628b8cae821b9f to 807588ebd019f99f2c3e9ed82207a1d5c1d50a67
Here's my version of Felix's branch, rebased on a recent develop branch. I think I preserved all the changes between the original version of his branch, and the more recent version from https://git.sagemath.org/sage.git/commit/?id=4846a73aad7cf7361c69a6ae43628b8cae821b9f So this removed many changes from the original branch, such as the change to how sage-maxima.lisp is located.
It would still be good to have a solution for that. But for now, this will let us look at what changes this branch does make (that are still relevant).
New commits:
8cb6f9f | unhardwire runtime paths in various cases
|
75cb534 | fix qepcadrc path
|
c484537 | don't rely on $SAGE_LOCAL/bin/python.
|
ffed9de | cython.py: don't rely in $SAGE_LOCAL
|
4278025 | python-sage: remove sage-location check
|
807588e | initialize SAGE_ETC (from environ) in sage-location before use
|
comment:43 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:44 Changed 6 years ago by
In src/sage/databases/sql_db.py
- from sage.env import SAGE_SHARE + from sage.misc.misc import GRAPHS_DATA_DIR
Go back to using sage.env
rather than sage.misc.misc
.
comment:45 Changed 6 years ago by
sage.env
is the wrong place for this. (not saying that, sage.misc.misc is any better, i hardly remember why it ended up there.).
GRAPHS_DATA_DIR certainly belongs to the part of code that uses graphs data... not to the core library (which i think env.py belongs to).
for the transition it might make sense to open a to be sorted
section in env.py... if you agree that doing it right[tm] is not within the scope of this ticket.
comment:46 Changed 6 years ago by
Historically, variables where stored in sage.misc.misc
. When we started to setup sage.env
as the (more comprehensive) variable store we deprecated the use of sage.misc.misc
.
I initially needed such a store in sage-on-gentoo anyway because my shell environment didn't contain them. It allowed me to do import sage.all
from the system python prompt amongst other things.
Now I am curious as to why sage.env
is not the right place for runtime variables - I would agree that now we should split the build time only variables somewhere else. But I am interested in your reasoning. Bear in mind that any answer from me will be several hours away (need to catch some zzzz).
comment:47 Changed 6 years ago by
Ah, I thought I replaced most of the sage.misc.misc
to with sage.env
bug I guess I missed that one.
I don't think there's really any problem with it being in sage.env
. If I were to do anything different it would be toward the goal of keeping environment variable definitions close to the code that actually uses them. It might be nice if a subpackage can have a sage.<foo>.env
describing variable specifically relevant to that sub-package, but still allow all variables to imported from sage.env
(lazily evaluated if necessary).
But I think that's all beyond the scope of this ticket--for now I think throwing everything in sage.env
is appropriate.
comment:48 follow-up: ↓ 75 Changed 6 years ago by
Now I am curious as to why sage.env is not the right place for runtime variables
because it breaks modularity. the core library should not have to know about extensions.
or the other way: there should be a way to extend sage/sagelib by merely installing something. then GRAPH_DATA (as well as some others listed in sage.env), should use that mechanism.
another remark: this is not just about environment variables. an extension should possibly add *anything*. but environment might be a good example, then generalize.
for now .. throwing everything in sage.env is appropriate.
agreed.
comment:49 Changed 6 years ago by
- Status changed from needs_review to needs_work
You cannot just unilaterally change the path for default.qepcadrc
, see 23
comment:50 Changed 6 years ago by
- Dependencies #15100 deleted
comment:51 Changed 6 years ago by
In pselect.pyx
, maybe it's better to use sys.executable
instead of os.path.join(SAGE_LOCAL, 'bin', 'python')
.
Because now the patch just replaces one hard-coded path by another.
comment:52 Changed 6 years ago by
I also don't agree with the removal of the test in src/sage/tests/cmdline.py
. The doctest has a function (it checks that sage-location
was run), you should not just remove it.
comment:53 Changed 6 years ago by
There is #15146 to deal with sage-location
and the test in src/sage/tests/cmdline.py
comment:54 Changed 6 years ago by
In sage.env
, use SAGE_LOCAL
instead of os.environ['SAGE_LOCAL']
.
comment:55 Changed 6 years ago by
- Dependencies set to #17254
The Singular changes will conflict with #17254.
comment:56 follow-up: ↓ 61 Changed 6 years ago by
Replace GAP_DATA_DIR
with GAP_DIR
. It's not a data dir, it's the installation directory of the whole GAP package.
comment:57 Changed 6 years ago by
In src/sage/geometry/lattice_polytope.py
, instead of assigning data_location = POLYTOPE_DATA_DIR
, it is cleaner to just use the variable POLYTOPE_DATA_DIR
directly.
comment:58 Changed 6 years ago by
Same comment in src/sage/schemes/elliptic_curves/ec_database.py
: you can shorten
from sage.env import ELLCURVE_DATA_DIR db = ELLCURVE_DATA_DIR data = os.path.join(db,'rank%s'%rank)
to
from sage.env import ELLCURVE_DATA_DIR data = os.path.join(ELLCURVE_DATA_DIR, 'rank%s'%rank)
comment:59 Changed 6 years ago by
These seem unused:
# used by cython.py _add_variable_or_fallback('SINGULAR_INCLUDEDIR', opj('$SAGE_LOCAL','include','singular')) _add_variable_or_fallback('FACTORY_INCLUDEDIR', opj('$SAGE_LOCAL','include','factory'))
comment:60 Changed 6 years ago by
Replace CONWAY_DATA_DIR
by CONWAY_POLYNOMIALS_DATA_DIR
because both the package and directory are named conway_polynomials
and not just conway
.
comment:61 in reply to: ↑ 56 ; follow-up: ↓ 62 Changed 6 years ago by
Replying to jdemeyer:
Replace
GAP_DATA_DIR
withGAP_DIR
. It's not a data dir, it's the installation directory of the whole GAP package.
thanks for the feedback.
what do you mean by 'installation directory of the whole GAP package'? the prefix?
note that the datadir (ought to be platform independent!) usually/generally/commonly not hardcoded to the prefix, hence the different name.
comment:62 in reply to: ↑ 61 ; follow-up: ↓ 63 Changed 6 years ago by
Replying to felixs:
what do you mean by 'installation directory of the whole GAP package'? the prefix?
GAP has an unusual installation process, it doesn't really have a prefix in the usual sense.
comment:63 in reply to: ↑ 62 ; follow-ups: ↓ 66 ↓ 67 Changed 6 years ago by
Replying to jdemeyer:
GAP has an unusual installation process, it doesn't really have a prefix in the usual sense.
so, it's not prefix (it does not seem relevant, does it?).
paths are subject to the conventions and standards on particular platforms (e.g. FSH), not to an "unusual installation process". package data usually goes to /usr/share/$packagename, and thats where i can find the gap data on my platform. hence, here, /usr/share/gap is GAP_DATA_DIR, and not GAP_DIR as you suggest.
(on a different platform it might be in a different place, but it's still "the place where the gap data is).
comment:64 Changed 6 years ago by
For reference, I believe Felix is referring to https://www.gnu.org/prep/standards/standards.html#Directory-Variables and https://www.gnu.org/software/automake/manual/automake.html#Uniform
comment:65 Changed 6 years ago by
yes. and not just GNU, see https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard
comment:66 in reply to: ↑ 63 ; follow-up: ↓ 68 Changed 6 years ago by
Replying to felixs:
package data usually goes to /usr/share/$packagename, and thats where i can find the gap data on my platform.
What do you mean with "gap data"?
Anyway, where your distro installs GAP does not matter. We are working on Sage here, and $SAGE_LOCAL/gap/latest
is the directory where GAP as a whole is installed. It's not a directory containing data files, so GAP_DATA_DIR
is a bad name.
comment:67 in reply to: ↑ 63 Changed 6 years ago by
Replying to felixs:
paths are subject to the conventions and standards on particular platforms (e.g. FSH), not to an "unusual installation process".
Accept the current reality: GAP has an "unusual installation process". We should use names which refer to the reality, not to some standard which doesn't apply for GAP.
comment:68 in reply to: ↑ 66 ; follow-up: ↓ 72 Changed 6 years ago by
Replying to jdemeyer:
What do you mean with "gap data"?
the platform independent data files shipped with ("installed by", "included in", "belonging to") the gap package.
comment:69 follow-up: ↓ 70 Changed 6 years ago by
I agree with Jeroen that the current practice of the GAP installation cannot be described in terms of standards. Let's call it "unusual" in writing.
Do we know how distributions like Debian handle GAP?
comment:70 in reply to: ↑ 69 Changed 6 years ago by
Do we know how distributions like Debian handle GAP?
on debian (and derivatives), you can find the platform independent gap data files in /usr/share/gap.
comment:71 Changed 6 years ago by
Jeroen, Felix: I don't have time to look into any specifics with GAP, but I would suggest that the directory variables are defined and named in a way that works both with
- with the status quo of how the Sage spkg installs GAP into
$SAGE_LOCAL
, - and with the way how a distribution such as Debian installs GAP into
/usr
.
On the other hand, if there's a wish to change the way how the Sage spkg install GAP, then that clearly belongs on a different ticket (please open it if it does not exist yet), not on this one.
comment:72 in reply to: ↑ 68 Changed 6 years ago by
comment:73 follow-ups: ↓ 74 ↓ 79 Changed 6 years ago by
GAP_DATA_DIR is a bad name.
no it's not. it specifies the location of the (platform indepentdent static, package specific) data shipped with the gap package. it's meant to be used to lead to exactly that. no more, no less.
i really don't see your problem here, Jeroen. can you explain what you want to use GAP_DIR for?
if you intend to use GAP)DIR for something other than the (platform indepentdent static, package specific) gap data, then you will break a standard FHS install. i know you are against standards, but why would you want that terrible implicit breakage here?
mkoeppe: no, nobody said the spkg needs to be changed, and in fact it does not have to.
comment:74 in reply to: ↑ 73 Changed 6 years ago by
Replying to felixs:
i know you are against standards
Please read https://en.wikipedia.org/wiki/Ad_hominem
I am not against standards, I am against denying reality. If the current reality is that GAP is not installed following the FHS standards, then we should just accept that fact.
comment:75 in reply to: ↑ 48 Changed 6 years ago by
Replying to felixs:
Now I am curious as to why sage.env is not the right place for runtime variables
because it breaks modularity. the core library should not have to know about extensions.
or the other way: there should be a way to extend sage/sagelib by merely installing something. then GRAPH_DATA (as well as some others listed in sage.env), should use that mechanism.
another remark: this is not just about environment variables. an extension should possibly add *anything*. but environment might be a good example, then generalize.
for now .. throwing everything in sage.env is appropriate.
agreed.
OK your new requirements and goals are different from the ones that lead to the emergence of env.py
. I can accept that we have to move on to a new state at some point in the future.
comment:76 follow-up: ↓ 77 Changed 6 years ago by
The current gap
situation is a hack at best. The problem is that gap
doesn't have a real install target. So everything is copied in a versioned folder inSAGE_LOCAL/gap/gap-$version
and then it is linked to SAGE_LOCAL/gap/latest
. Clean in that it is all in its own container that you can delete afterwards.
I would imagine that debian does something like I do in Gentoo. Shoves most of the stuff under /usr/lib{,64}/gap
with appropriate links or script in standard location pointing there. In that scheme GAP_DATA_DIR is /usr/lib{,64}/gap/pkg
although some of the databases live under /usr/lib{,64}/gap
directly.
There is a lot to do upstream to get gap
to install nicely in any kind of fashion. In my latest ebuild I have been quite drastic compared to what sage does or previous version of my own ebuilds. So for the foreseeable future gap
will remain a hack and I think GAP_DIR
is more apt than GAP_DATA_DIR
.
comment:77 in reply to: ↑ 76 Changed 6 years ago by
Replying to fbissey:
I would imagine that debian does something like I do in Gentoo.
with all do respect, you got it wrong.
on debian, the (platform indepentdent static, package specific) gap data goes to /usr/share/gap
Shoves most of the stuff under
/usr/lib{,64}/gap
with appropriate links or script in standard location pointing there. In that scheme GAP_DATA_DIR is/usr/lib{,64}/gap/pkg
although some of the databases live under/usr/lib{,64}/gap
directly.
not quite. /usr/lib is for platform dependent stuff. that's why you need lib{32,64} but only shared (not shared{32,64}). GAP_DATA_DIR is about *platform independent data*.
There is a lot to do upstream to get
gap
to install nicely in any kind of fashion.
wrong again. you can install gap any way you like, despite the broken build/install system
I think
GAP_DIR
is more apt thanGAP_DATA_DIR
.
now with three wrong premises, you end up on the wrong track.
instead, you could (plan to) fix the gap ebuild to adhere to FHS, as most other gentoo packages do.
Replying to Jeroen
I am not against standards (... but)
i feel the need for a gowdin's style law for this sentence, but it's not there. so i will better stop posting here now.
comment:78 follow-up: ↓ 103 Changed 6 years ago by
I certainly could improve things, thanks for pointing stuff out [although using /usr/share/gap means patching, I guess debian has the patch I am thinking about]. The main point here is that installing gap in a proper way is a lot of hand holding. debian and gentoo have a proper package management system - which track files belonging to a package so upgrading or otherwise changing the package version is clean.
sagemath on the other hand only installs and "uninstall" manually where we can/have to [I guess there are pip
packages now and we know what happened with those in another ticket ;) ]. What I am getting at, is that sagemath's packaging system is not sophisticated enough to deal with the hand holding when changing version. Lots of work.
Feel free to improve gap
's installation (without forgetting to make provision for upgrade) in another ticket.
Once it is done the discussion between GAP_DATA_DIR
and GAP_DIR
will be non-sensical. Should we wait until this is ready or should we move on with the current situation?
comment:79 in reply to: ↑ 73 ; follow-up: ↓ 80 Changed 6 years ago by
Replying to felixs:
GAP_DATA_DIR is a bad name.
no it's not. it specifies the location of the (platform indepentdent static, package specific) data shipped with the gap package. it's meant to be used to lead to exactly that. no more, no less.
I agree with Felix here. I don't care how gap happens to be installed in Sage. The use that variable is to find where the GAP data lives (which in Sage, which I guess obeys GAP's "unusual" installation, happens to be the directory where all of GAP is installed). On a different platform the two are not necessarily the same.
comment:80 in reply to: ↑ 79 ; follow-up: ↓ 82 Changed 6 years ago by
Replying to embray:
The use [of] that variable is to find where the GAP data lives
Do you have evidence for this claim? Because this statement is the crux of the discussion.
Edit: if the above claim is indeed correct, I totally agree that GAP_DATA_DIR
is a good name.
comment:81 Changed 6 years ago by
Anyway, back to reality: $SAGE_LOCAL/gap/latest
is what GAP calls a GAP Root Directory. It seems to be used for everything, not just "data files".
comment:82 in reply to: ↑ 80 ; follow-up: ↓ 83 Changed 6 years ago by
Replying to jdemeyer:
Replying to embray:
The use [of] that variable is to find where the GAP data lives
Do you have evidence for this claim? Because this statement is the crux of the discussion.
Edit: if the above claim is indeed correct, I totally agree that
GAP_DATA_DIR
is a good name.
It seems to me all this argument might be for not much then. It seems to me all it's really used for is in the utility function gap_root()
, which itself is not actually used anywhere in Sage (which is not to say nobody uses it for something).
Perhaps we can call it GAP_ROOT_DIR
for now and leave it at that. Distros that try to break GAP up into a standard hierarchy would have to have their own definition of what GAP_ROOT_DIR
means I guess.
Agreed with all of Jeroen's other comments from before the GAP_DIR
argument--I don't at all understand the issue with default.qepcadrc
but I'll have a closer look.
comment:83 in reply to: ↑ 82 Changed 6 years ago by
Replying to embray:
It seems to me all it's really used for is in the utility function
gap_root()
, which itself is not actually used anywhere in Sage (which is not to say nobody uses it for something).
It is used to initialize libGAP. From src/sage/libs/gap/util.pyx
:
cdef initialize(): """ Initialize the GAP library, if it hasn't already been initialized. It is safe to call this multiple times. TESTS:: sage: libgap(123) # indirect doctest 123 """ global _gap_is_initialized if _gap_is_initialized: return # Define argv and environ variables, which we will pass in to # initialize GAP. Note that we must pass define the memory pool # size! cdef char* argv[14] argv[0] = "sage" argv[1] = "-l" s = gap_root() argv[2] = s
comment:84 Changed 6 years ago by
That makes sense, obviously. Not sure how my grepping missed that.
comment:85 follow-up: ↓ 86 Changed 6 years ago by
- Cc dimpase vbraun kcrisman mmarco nbruin added
Cc'ing some people who worked on libgap (#6391) in the hope that we can settle the GAP_DIR / GAP_ROOT_DIR / GAP_DATA_DIR question and move this ticket forward.
comment:86 in reply to: ↑ 85 Changed 6 years ago by
comment:87 follow-up: ↓ 88 Changed 6 years ago by
We are looking for the correct naming of the variable that holds the directory passed to libgap at initialization time. The naming should make sense both for GAP's traditional (upstream) installation and for Debian's or other distributions' cleaned up installation.
comment:88 in reply to: ↑ 87 Changed 6 years ago by
Replying to mkoeppe:
We are looking for the correct naming of the variable that holds the directory passed to libgap at initialization time. The naming should make sense both for GAP's traditional (upstream) installation and for Debian's or other distributions' cleaned up installation.
libGAP is a purely Sage affair, there is no real upstream, so there is no "tradition" in this sense.
comment:89 follow-ups: ↓ 90 ↓ 91 Changed 6 years ago by
but libgap does seem to depend on GAP's installation, no?
comment:90 in reply to: ↑ 89 Changed 6 years ago by
Replying to mkoeppe:
but libgap does seem to depend on GAP's installation, no?
yes it does, sure, as enshrined in build/pkgs/libgap/dependencies
comment:91 in reply to: ↑ 89 ; follow-up: ↓ 96 Changed 6 years ago by
Replying to mkoeppe:
but libgap does seem to depend on GAP's installation, no?
It absolutely does. In fact it needs to know where gap packages are if it want to use them. So it really need what gap calls GAP_ROOT_DIR
.
comment:92 Changed 6 years ago by
The Debian/Ubuntu? gap package splits the gap installation into /usr/lib/gap
and /usr/share/gap
.
root@2f38a5ed7694:/src# ls -l /usr/lib/gap total 8 drwxr-xr-x 2 root root 4096 Oct 27 23:24 bin lrwxrwxrwx 1 root root 17 Jun 18 2015 src -> ../../include/gap -rw-r--r-- 1 root root 110 Jun 18 2015 sysinfo.gap root@2f38a5ed7694:/src# ls -l /usr/share/gap total 56 drwxr-xr-x 5 root root 4096 Oct 27 23:24 doc drwxr-xr-x 2 root root 4096 Oct 27 23:24 etc drwxr-xr-x 2 root root 4096 Oct 27 23:24 grp drwxr-xr-x 2 root root 20480 Oct 27 23:24 lib drwxr-xr-x 3 root root 4096 Oct 27 23:24 pkg drwxr-xr-x 3 root root 4096 Oct 27 23:24 prim drwxr-xr-x 4 root root 4096 Oct 27 23:24 small lrwxrwxrwx 1 root root 25 Jun 18 2015 sysinfo.gap -> ../../lib/gap/sysinfo.gap drwxr-xr-x 2 root root 12288 Oct 27 23:24 trans
I did a quick check of building libgap using the gap package on Ubuntu. The relevant directory for libgap seems to be /usr/share/gap
.
comment:93 Changed 6 years ago by
- Branch changed from u/embray/ticket-15105 to u/mkoeppe/ticket-15105
comment:94 Changed 6 years ago by
- Commit changed from 807588ebd019f99f2c3e9ed82207a1d5c1d50a67 to 437827b89af4dcf6842dd5d343489247fb779b16
- Status changed from needs_work to needs_review
comment:95 Changed 6 years ago by
- Status changed from needs_review to needs_work
Many of my comments starting with 56 have not been taken into account.
comment:96 in reply to: ↑ 91 Changed 6 years ago by
Replying to fbissey:
It absolutely does. In fact it needs to know where gap packages are if it want to use them. So it really need what gap calls
GAP_ROOT_DIR
.
GAP_ROOT_DIR
would indeed be a much better name than the misleading GAP_DATA_DIR
.
comment:97 Changed 6 years ago by
There are also doctest failures (see patchbot).
Why was this ticket set to needs_review?
comment:98 Changed 6 years ago by
Someone insists on importing variables from sage.misc.misc
instead of sage.env
ergo doctest failures.
comment:99 Changed 6 years ago by
Right, I set needs_review
by mistake.
comment:100 Changed 6 years ago by
- Commit changed from 437827b89af4dcf6842dd5d343489247fb779b16 to a80df367fbddcbcb7e4df637efdc38e5a2e22eff
Branch pushed to git repo; I updated commit sha1. New commits:
5db9663 | lattice_polytope: Use POLYTOPE_DATA_DIR directly
|
b011153 | ec_database: Use ELLCURVE_DATA_DIR directly
|
171e139 | Rename CONWAY_DATA_DIR to CONWAY_POLYNOMIALS_DATA_DIR
|
a222fc9 | Rename GAP_DATA_DIR to GAP_ROOT_DIR
|
a80df36 | Import GRAPHS_DATA_DIR from sage.env, not sage.misc.misc
|
comment:101 Changed 6 years ago by
- Commit changed from a80df367fbddcbcb7e4df637efdc38e5a2e22eff to cf810a2203c8b6bb75f2e2539fe21bb3187b9558
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
41b40fd | unhardwire runtime paths in various cases
|
6e0738e | lattice_polytope: Use POLYTOPE_DATA_DIR directly
|
82f9882 | ec_database: Use ELLCURVE_DATA_DIR directly
|
0d56551 | Rename CONWAY_DATA_DIR to CONWAY_POLYNOMIALS_DATA_DIR
|
f7a462e | Rename GAP_DATA_DIR to GAP_ROOT_DIR
|
cf810a2 | Import GRAPHS_DATA_DIR from sage.env, not sage.misc.misc
|
comment:102 Changed 6 years ago by
- Commit changed from cf810a2203c8b6bb75f2e2539fe21bb3187b9558 to 65a00a5b5a724fab0685def2ef103a299c740ac4
comment:103 in reply to: ↑ 78 Changed 6 years ago by
comment:104 follow-up: ↓ 106 Changed 6 years ago by
- Status changed from needs_work to needs_review
The present ticket now passes doctests (on my machine), and I believe I have taken care of the various comments.
Needs review.
comment:105 Changed 6 years ago by
- Dependencies #17254 deleted
comment:106 in reply to: ↑ 104 Changed 6 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to needs_work
Replying to mkoeppe:
The present ticket now passes doctests (on my machine), and I believe I have taken care of the various comments.
Two more details:
- in
src/sage/env.py
, removeUNAME = os.uname()[0]
as UNAME
is actually one of the standard variables.
- In
src/sage/libs/gap/util.pyx
, removegapdir = GAP_ROOT_DIR
and just useGAP_ROOT_DIR
instead ofgapdir
.
If you fix these and all tests pass, then this can be set to positive_review.
comment:107 Changed 6 years ago by
- Commit changed from 65a00a5b5a724fab0685def2ef103a299c740ac4 to 45c5dd010981c26d99773e97b9589a305326b90b
comment:108 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:109 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:110 follow-up: ↓ 111 Changed 6 years ago by
thank you guys for making that happen.
comment:111 in reply to: ↑ 110 Changed 6 years ago by
Replying to felixs:
thank you guys for making that happen.
Well, we did postpone all controversial changes to other tickets :-)
comment:112 Changed 6 years ago by
Let me mention one more follow-up: #21821
comment:113 follow-up: ↓ 114 Changed 6 years ago by
Now that it has made it onto Volker's branch and I can see easily how it interact with sage-on-gentoo, I feel sorry about all that GAP_ROOT_DIR
business.
We should just have eliminated it because it is totally unnecessary. The variable is only used in sage/libs/gap/util.pyx
in the function gap_root
which basically return the value of GAP_ROOT_DIR
def gap_root(): """ Find the location of the GAP root install which is stored in the gap startup script. EXAMPLES:: sage: from sage.libs.gap.util import gap_root sage: gap_root() # random output '/home/vbraun/opt/sage-5.3.rc0/local/gap/latest' """ import os.path if os.path.exists(GAP_ROOT_DIR): return GAP_ROOT_DIR print('The gap-4.5.5.spkg (or later) seems to be not installed!') gap_sh = open(os.path.join(SAGE_LOCAL, 'bin', 'gap')).read().splitlines() gapdir = filter(lambda dir:dir.strip().startswith('GAP_DIR'), gap_sh)[0] gapdir = gapdir.split('"')[1] gapdir = gapdir.replace('$SAGE_LOCAL', SAGE_LOCAL) return gapdir
So if GAP_ROOT_DIR
is not defined, a mwarning is issued and then the gap
script is looked over for GAP_DIR
which gives the right result both in Gentoo [sage-on-gentoo relies solely on the automated procedure] and in vanilla sage. Not sure about debian, I must say.
But I feel we were silly because the automated procedure works well so we don't really need the variable. And if we decide to use the variable, what is the point of keeping the function gap_root
? Apart from searching for all the instances of gap_root
to replace them that is.
Anyway, that's a potential clean up for another ticket.
comment:114 in reply to: ↑ 113 Changed 6 years ago by
Replying to fbissey:
[...] I feel sorry about all that
GAP_ROOT_DIR
business.We should just have eliminated it because it is totally unnecessary. The variable is only used in
sage/libs/gap/util.pyx
in the functiongap_root
which basically return the value ofGAP_ROOT_DIR
[...] So ifGAP_ROOT_DIR
is not defined, a mwarning is issued and then thegap
script is looked over forGAP_DIR
which gives the right result both in Gentoo [sage-on-gentoo relies solely on the automated procedure] and in vanilla sage.
Yes, I saw this code. I think it does not belong into sagelib proper, but we should move it to compile-time configuration of sagelib (which would set the GAP_ROOT_DIR variable). So I do think that introducing the GAP_ROOT_DIR variable was a good step.
comment:115 follow-up: ↓ 121 Changed 6 years ago by
A lot of stuff should be moved to "configuration" time proper. Definitely for another day.
But is there a point in having a variable when you can just query its value from the application itself? Plus that means we can further decouple gap
from sagelib
. We don't need to know anything about where gap
is really installed, we get the right value so long as gap
is in the PATH
. In short eliminating that variable makes it easier to switch to a system install of gap
(cross fingers about debian).
comment:116 Changed 6 years ago by
OK at the moment we still need to know where the gap
executable is located to populate gap_sh
but that could be dealt with.
comment:117 Changed 6 years ago by
I wouldn't call parsing that script "querying the application"...
comment:118 Changed 6 years ago by
And, really, actually it belongs into libgap configuration.
comment:119 Changed 6 years ago by
Have to agree, it is a libgap
problem. Once sorted in libgap
it is all moot.
comment:120 Changed 6 years ago by
I recently created a pull request for libgap in this direction: https://bitbucket.org/vbraun/libgap/pull-requests/9
comment:121 in reply to: ↑ 115 ; follow-up: ↓ 123 Changed 6 years ago by
Replying to fbissey:
A lot of stuff should be moved to "configuration" time proper. Definitely for another day.
But is there a point in having a variable when you can just query its value from the application itself?
Yes. I can think of at least two reasons:
1) Helpful for some system (not that I have one in mind) where the search code doesn't work--it's always kind to provide some way to configure an alternate path.
2) For testing against alternate GAP installs.
comment:122 Changed 6 years ago by
- Branch changed from u/mkoeppe/ticket-15105 to 45c5dd010981c26d99773e97b9589a305326b90b
- Resolution set to fixed
- Status changed from positive_review to closed
comment:123 in reply to: ↑ 121 Changed 6 years ago by
- Commit 45c5dd010981c26d99773e97b9589a305326b90b deleted
Replying to embray:
Replying to fbissey:
A lot of stuff should be moved to "configuration" time proper. Definitely for another day.
But is there a point in having a variable when you can just query its value from the application itself?
Yes. I can think of at least two reasons:
1) Helpful for some system (not that I have one in mind) where the search code doesn't work--it's always kind to provide some way to configure an alternate path.
2) For testing against alternate GAP installs.
1) a good case is probably cross compilation of some kind, where things will be available at runtime but not when you build.
2) well I was thinking of asking gap
directly (I have tested this) so the only requirement is for the gap
executable to be in the PATH
. However technically there is more than one valid path that should be looked up and libgap only look at one (the default system one).
Removed the ipython-sage-location check, as it involves
$SAGE_LOCAL
. Thus dependency on #15100.