Opened 6 years ago

Closed 3 years ago

Last modified 3 years ago

#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) 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 6 years ago by felixs

  • Branch set to u/felixs/runtime_paths
  • Commit set to 2cffff84404e4179adc19860b29c5bae9ed80742

comment:2 Changed 6 years ago by felixs

  • Dependencies set to #15100

Removed the ipython-sage-location check, as it involves $SAGE_LOCAL. Thus dependency on #15100.

comment:3 Changed 6 years ago by jondo

  • Cc jondo added

comment:4 Changed 6 years ago by git

  • 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 6 years ago by git

  • 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 6 years ago by felixs

  • Status changed from new to needs_review

comment:7 Changed 6 years ago by git

  • 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: Changed 6 years ago by jdemeyer

  • 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 6 years ago by felixs

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 6 years ago by fbissey

  • Cc fbissey added

comment:11 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:12 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:13 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 3 years ago by mkoeppe

  • Cc mkoeppe added

comment:15 Changed 3 years ago by mkoeppe

This patch needs rebasing

comment:16 Changed 3 years ago by git

  • Commit changed from 0b9cb04e432036e47e1fb735f2044b73e4f0ae58 to 12f698526f1385923acc5c1b2b0c855ea908e3c1

Branch pushed to git repo; I updated commit sha1. New commits:

faa99e7Merge branch 'master' into runtime_paths
12f6985refresh more runtime_paths commits

comment:17 Changed 3 years ago by git

  • Commit changed from 12f698526f1385923acc5c1b2b0c855ea908e3c1 to 8149a85cae9ea41685a0954a8d5b275caef16807

Branch pushed to git repo; I updated commit sha1. New commits:

8149a85Merge 'develop' into runtime_paths

comment:18 Changed 3 years ago by felixs

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: Changed 3 years ago by fbissey

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 3 years ago by git

  • Commit changed from 8149a85cae9ea41685a0954a8d5b275caef16807 to 4846a73aad7cf7361c69a6ae43628b8cae821b9f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4846a73merge develop into runtime_paths

comment:21 in reply to: ↑ 19 Changed 3 years ago by felixs

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 3 years ago by mkoeppe

Instead of sage.misc.misc, shouldn't it use sage.env?

comment:23 Changed 3 years ago by fbissey

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

Last edited 3 years ago by fbissey (previous) (diff)

comment:24 follow-ups: Changed 3 years ago by 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?

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.

#20382

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 3 years ago by mkoeppe

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: Changed 3 years ago by fbissey

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.

#20382

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 3 years ago by felixs

Replying to fbissey:

i appreciate your clear reply. but i'm out of time. maybe next week.

comment:28 Changed 3 years ago by jdemeyer

About sage-started.txt: it has two purposes:

  1. Check that Sage starts.
  1. Initialize any files which should be initialized at build time (this includes running sage-location and generating .pyc files). See #11926.

comment:29 Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by felixs

sage_started is now in #21510

comment:32 Changed 3 years ago by mkoeppe

OK, best to remove anything about sage_started from the present ticket's branch then.

comment:33 Changed 3 years ago by felixs

sage_location now is #21511.

comment:34 Changed 3 years ago by mkoeppe

  • Milestone changed from sage-6.4 to sage-7.5

comment:35 Changed 3 years ago by mkoeppe

See also #21591.

comment:36 Changed 3 years ago by embray

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: Changed 3 years ago by embray

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: Changed 3 years ago by 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?)

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 3 years ago by fbissey

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: Changed 3 years ago by embray

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 3 years ago by felixs

Replying to embray:

.. if it's ok with felixs.

sure it is. please do!

comment:42 Changed 3 years ago by embray

  • Authors changed from Felix Salfelder to Felix Salfelder, Erik Bray
  • 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:

8cb6f9funhardwire runtime paths in various cases
75cb534fix qepcadrc path
c484537don't rely on $SAGE_LOCAL/bin/python.
ffed9decython.py: don't rely in $SAGE_LOCAL
4278025python-sage: remove sage-location check
807588einitialize SAGE_ETC (from environ) in sage-location before use

comment:43 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:44 Changed 3 years ago by fbissey

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 3 years ago by felixs

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 3 years ago by fbissey

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 3 years ago by embray

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: Changed 3 years ago by 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.

comment:49 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

You cannot just unilaterally change the path for default.qepcadrc, see 23

comment:50 Changed 3 years ago by jdemeyer

  • Dependencies #15100 deleted

comment:51 Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

There is #15146 to deal with sage-location and the test in src/sage/tests/cmdline.py

comment:54 Changed 3 years ago by jdemeyer

In sage.env, use SAGE_LOCAL instead of os.environ['SAGE_LOCAL'].

comment:55 Changed 3 years ago by jdemeyer

  • Dependencies set to #17254

The Singular changes will conflict with #17254.

comment:56 follow-up: Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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: Changed 3 years ago by felixs

Replying to jdemeyer:

Replace GAP_DATA_DIR with GAP_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: Changed 3 years ago by jdemeyer

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: Changed 3 years ago by felixs

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:66 in reply to: ↑ 63 ; follow-up: Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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: Changed 3 years ago by felixs

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: Changed 3 years ago by mkoeppe

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 3 years ago by felixs

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 3 years ago by mkoeppe

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 3 years ago by jdemeyer

Replying to felixs:

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.

Right. With that definition, $SAGE_LOCAL/gap/latest is not the GAP data dir so GAP_DATA_DIR is a bad name.

comment:73 follow-ups: Changed 3 years ago by 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 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 3 years ago by jdemeyer

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 3 years ago by fbissey

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: Changed 3 years ago by fbissey

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 3 years ago by felixs

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 than GAP_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: Changed 3 years ago by fbissey

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: Changed 3 years ago by embray

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.

Last edited 3 years ago by embray (previous) (diff)

comment:80 in reply to: ↑ 79 ; follow-up: Changed 3 years ago by 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.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:81 Changed 3 years ago by jdemeyer

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: Changed 3 years ago by embray

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 3 years ago by jdemeyer

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 3 years ago by embray

That makes sense, obviously. Not sure how my grepping missed that.

comment:85 follow-up: Changed 3 years ago by mkoeppe

  • 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 3 years ago by dimpase

Replying to mkoeppe:

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.

OK, so what is your proposal in this regard? GAP suffers from the same "missing install target" illness as Sage....

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

comment:88 in reply to: ↑ 87 Changed 3 years ago by dimpase

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: Changed 3 years ago by mkoeppe

but libgap does seem to depend on GAP's installation, no?

comment:90 in reply to: ↑ 89 Changed 3 years ago by dimpase

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: Changed 3 years ago by fbissey

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 3 years ago by mkoeppe

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 3 years ago by mkoeppe

  • Branch changed from u/embray/ticket-15105 to u/mkoeppe/ticket-15105

comment:94 Changed 3 years ago by mkoeppe

  • Authors changed from Felix Salfelder, Erik Bray to Felix Salfelder, Erik Bray, Matthias Koeppe
  • Commit changed from 807588ebd019f99f2c3e9ed82207a1d5c1d50a67 to 437827b89af4dcf6842dd5d343489247fb779b16
  • Status changed from needs_work to needs_review

I've split out some commits to #21783 and #21784.

Rebased the remaining commits on top of 7.5.beta0.


New commits:

437827bunhardwire runtime paths in various cases

comment:95 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Many of my comments starting with 56 have not been taken into account.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:96 in reply to: ↑ 91 Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

There are also doctest failures (see patchbot).

Why was this ticket set to needs_review?

comment:98 Changed 3 years ago by fbissey

Someone insists on importing variables from sage.misc.misc instead of sage.env ergo doctest failures.

comment:99 Changed 3 years ago by mkoeppe

Right, I set needs_review by mistake.

comment:100 Changed 3 years ago by git

  • Commit changed from 437827b89af4dcf6842dd5d343489247fb779b16 to a80df367fbddcbcb7e4df637efdc38e5a2e22eff

Branch pushed to git repo; I updated commit sha1. New commits:

5db9663lattice_polytope: Use POLYTOPE_DATA_DIR directly
b011153ec_database: Use ELLCURVE_DATA_DIR directly
171e139Rename CONWAY_DATA_DIR to CONWAY_POLYNOMIALS_DATA_DIR
a222fc9Rename GAP_DATA_DIR to GAP_ROOT_DIR
a80df36Import GRAPHS_DATA_DIR from sage.env, not sage.misc.misc

comment:101 Changed 3 years ago by git

  • Commit changed from a80df367fbddcbcb7e4df637efdc38e5a2e22eff to cf810a2203c8b6bb75f2e2539fe21bb3187b9558

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

41b40fdunhardwire runtime paths in various cases
6e0738elattice_polytope: Use POLYTOPE_DATA_DIR directly
82f9882ec_database: Use ELLCURVE_DATA_DIR directly
0d56551Rename CONWAY_DATA_DIR to CONWAY_POLYNOMIALS_DATA_DIR
f7a462eRename GAP_DATA_DIR to GAP_ROOT_DIR
cf810a2Import GRAPHS_DATA_DIR from sage.env, not sage.misc.misc

comment:102 Changed 3 years ago by git

  • Commit changed from cf810a2203c8b6bb75f2e2539fe21bb3187b9558 to 65a00a5b5a724fab0685def2ef103a299c740ac4

Branch pushed to git repo; I updated commit sha1. New commits:

a2c3798GraphClasses: fixup use of GRAPHS_DATA_DIR
65a00a5Remove unused variables SINGULAR_INCLUDEDIR, FACTORY_INCLUDEDIR

comment:103 in reply to: ↑ 78 Changed 3 years ago by mkoeppe

Replying to fbissey:

using /usr/share/gap means patching, I guess debian has the patch I am thinking about

I've created ticket #21796 for adopting Debian's layout of GAP.

comment:104 follow-up: Changed 3 years ago by mkoeppe

  • 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 3 years ago by jdemeyer

  • Dependencies #17254 deleted

comment:106 in reply to: ↑ 104 Changed 3 years ago by jdemeyer

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

  1. in src/sage/env.py, remove
    UNAME = os.uname()[0]
    

as UNAME is actually one of the standard variables.

  1. In src/sage/libs/gap/util.pyx, remove gapdir = GAP_ROOT_DIR and just use GAP_ROOT_DIR instead of gapdir.

If you fix these and all tests pass, then this can be set to positive_review.

comment:107 Changed 3 years ago by git

  • Commit changed from 65a00a5b5a724fab0685def2ef103a299c740ac4 to 45c5dd010981c26d99773e97b9589a305326b90b

Branch pushed to git repo; I updated commit sha1. New commits:

ae20d15Remove duplicate assignment to UNAME
45c5dd0gap_root: slight simplification

comment:108 Changed 3 years ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:109 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:110 follow-up: Changed 3 years ago by felixs

thank you guys for making that happen.

comment:111 in reply to: ↑ 110 Changed 3 years ago by jdemeyer

Replying to felixs:

thank you guys for making that happen.

Well, we did postpone all controversial changes to other tickets :-)

comment:112 Changed 3 years ago by jdemeyer

Let me mention one more follow-up: #21821

comment:113 follow-up: Changed 3 years ago by fbissey

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 3 years ago by mkoeppe

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 function gap_root which basically return the value of GAP_ROOT_DIR [...] 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.

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: Changed 3 years ago by 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? 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 3 years ago by fbissey

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 3 years ago by mkoeppe

I wouldn't call parsing that script "querying the application"...

comment:118 Changed 3 years ago by mkoeppe

And, really, actually it belongs into libgap configuration.

comment:119 Changed 3 years ago by fbissey

Have to agree, it is a libgap problem. Once sorted in libgap it is all moot.

comment:120 Changed 3 years ago by mkoeppe

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: Changed 3 years ago by 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.

comment:122 Changed 3 years ago by vbraun

  • 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 3 years ago by fbissey

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

Note: See TracTickets for help on using tickets.