Opened 2 years ago

Closed 20 months ago

Last modified 20 months ago

#25309 closed enhancement (fixed)

Make more paths configurable

Reported by: gh-timokau Owned by:
Priority: major Milestone: sage-8.4
Component: distribution Keywords:
Cc: saraedum Merged in:
Authors: Timo Kaufmann Reviewers: Julian Rüth
Report Upstream: N/A Work issues:
Branch: 97f06fd (Commits) Commit:
Dependencies: Stopgaps:

Description

While packaging sage for nixos, I needed to configure some paths in addition to the already provided environment variables. Specifically I added COMBINATORIAL_DESIGN_DATA_DIR, CREMONA_MINI_DATA_DIR, CREMONA_LARGE_DATA_DIR, JMOL_DIR, JSMOL_DIR, MATHJAX_DIR andTHREEJS_DIR.

I think those changes should be included in the sage distribution.

Change History (32)

comment:1 Changed 2 years ago by gh-timokau

  • Branch changed from gh-timokau/spkg-paths to /u/gh-timokau/spkg-paths
  • Commit set to 138929719d9fad77106c44e68ab37bbe171ebc6a

comment:2 Changed 2 years ago by gh-timokau

  • Authors set to Timo Kaufmann
  • Commit changed from 138929719d9fad77106c44e68ab37bbe171ebc6a to 2d2da82bdbc71cbe816eaa47a9d8d206d2cdf631
  • Status changed from new to needs_review

New commits:

2d2da82Make more paths configurable

comment:3 Changed 2 years ago by jdemeyer

  • Cc saraedum added

comment:4 Changed 23 months ago by gh-timokau

  • Commit changed from 2d2da82bdbc71cbe816eaa47a9d8d206d2cdf631 to 670f6989f392ef9ea5074a34418907dcc11a0703

I've added one more patch that makes the location of maxima.fas configurable. Unfortunately ecl doesn't have an equivalent to PYTHON_PATH, so we have to explicitly provide the path. ECLLIB is already set in sage-env.


New commits:

670f698Make maxima.fas location configurable

comment:5 follow-up: Changed 23 months ago by saraedum

  • Commit changed from 670f6989f392ef9ea5074a34418907dcc11a0703 to 97f06fddee920399d4fcda65aa9b0925774aec69

gh-timokau, nice work. It'd be great to have more of these things configurable.

I wonder what should be the general approach here. I guess it would be good to have an example that we can follow in the future for other SPGKs:

Should some things be set at ./configure-time and not be modifiable through environment variables? What should be configurable through environment variables? How should these be named? Should they all be SAGE_*? Or are they sufficiently general that it makes sense to drop the SAGE_ prefix?


New commits:

a861514Make more paths configurable
97f06fdMake maxima.fas location configurable

comment:6 in reply to: ↑ 5 Changed 23 months ago by gh-timokau

For the nix package I stopped using make and ./configure altogether. Currently they (especially the makefile) just complicate things and provide little value if you want to use system packages.

If the configure script would be able to *reliably* determine these values, that'd be nice. Especially if you could then run sage without importing a shell file (and thus using it as a normal python package). I know env.py is intended for this, but that doesn't work as soon as some pats are non-standard.

For now I picked the least intrusive way to be able to package sage with as little patching as possible.

comment:7 Changed 23 months ago by saraedum

  • Reviewers set to Julian Rüth

I see. You are following the existing pattern so these changes seem alright to me (though I do not think it's a good pattern.)

comment:8 Changed 23 months ago by saraedum

Unfortunately our patchbot is not going to test this and I would prefer to see that this does not break anything.

It would probably be good to merge in https://trac.sagemath.org/ticket/24854 so this gets tested on gitlab.

comment:9 Changed 21 months ago by gh-timokau

It seems like #24854 is a pretty complex ticket. Do we really need to block this on that?

comment:10 Changed 21 months ago by saraedum

When I proposed to use #24854, I had the impression that that would be merged soon anyway. I am not so sure about that anymore. So, did you run a full build and doctests for this one?

comment:11 Changed 21 months ago by gh-timokau

I'm pretty sure I did test this with make ptest. I definitely tested this multiple times with the nix package, without any test failures.

Unfortunately I can't reproduce make ptest right now because the sage-the-distribution build always fails (with or without this patch) with some segfault in the python build.

Last edited 21 months ago by gh-timokau (previous) (diff)

comment:12 Changed 21 months ago by saraedum

Ok. Feel free to set this to positive review if you are confident that this still works.

comment:13 Changed 21 months ago by gh-timokau

  • Status changed from needs_review to positive_review

My build failure was due to https://bugs.python.org/issue33374, which is already fixed in the latest beta (by updating python).

All tests pass.

comment:14 Changed 21 months ago by vbraun

  • Status changed from positive_review to needs_work

Invalid refspec '/u/gh-timokau/spkg-paths'

comment:15 Changed 21 months ago by saraedum

  • Branch changed from /u/gh-timokau/spkg-paths to u/gh-timokau/spkg-paths

comment:16 Changed 21 months ago by saraedum

  • Status changed from needs_work to positive_review

comment:17 Changed 20 months ago by vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

comment:18 Changed 20 months ago by vbraun

  • Branch changed from u/gh-timokau/spkg-paths to 97f06fddee920399d4fcda65aa9b0925774aec69
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 follow-up: Changed 20 months ago by fbissey

  • Commit 97f06fddee920399d4fcda65aa9b0925774aec69 deleted

I will probably have to do a follow up on this. I am not sure yet what form it will take but for sage-on-gentoo I am basically reverting anything that rely on a definition being in sage-env.

That was a wrong move. A long time ago env.py didn't exist and sage-env was the only place sage variables were defined. To get sage working in all conditions I basically added essential variables to the default shell environment. Then I was requested not to do that. That's when for the first time I started something like env.py in sage - there was some stuff in misc/misc.py but that was clearly a second thought and very incomplete. The idea was thought of in parallel by a sage-dev and we ended collaborating on what became env.py.

One of the main advantages of using env.py is that I can import sage from a python shell and even if sage-env has not been run it will just work. Boy, did you destroy that.

So I think most of the patch is good even it means some work for me because I don't have the same default, in one case it probably uncovered a bug which strangely was silent for years in calling jmol. But where you use stuff from sage-env I see a failure. You basically don't go far enough.

comment:20 in reply to: ↑ 19 Changed 20 months ago by gh-timokau

Replying to fbissey:

I will probably have to do a follow up on this. I am not sure yet what form it will take but for sage-on-gentoo I am basically reverting anything that rely on a definition being in sage-env.

I didn't change any defaults, I didn't even touch the sage-env file. I only added configurability, keeping all the defaults intact. So I'm not sure what you mean by this.

That was a wrong move.

What was a wrong move?

A long time ago env.py didn't exist and sage-env was the only place sage variables were defined. To get sage working in all conditions I basically added essential variables to the default shell environment. Then I was requested not to do that. That's when for the first time I started something like env.py in sage - there was some stuff in misc/misc.py but that was clearly a second thought and very incomplete. The idea was thought of in parallel by a sage-dev and we ended collaborating on what became env.py. So I'm not sure what you mean by this.

One of the main advantages of using env.py is that I can import sage from a python shell and even if sage-env has not been run it will just work.

Thank you for your work on that! I agree that keeping all within python is a strictly better solution. However as I see it env.py at the moment doesn't really solve that, since it just provides access to environment variables or defaults if they are not set. Better would maybe be a simple configuration file that can be parsed by env.py. But that was out of scope of this ticket.

Boy, did you destroy that.

So I think most of the patch is good even it means some work for me because I don't have the same default, in one case it probably uncovered a bug which strangely was silent for years in calling jmol. But where you use stuff from sage-env I see a failure. You basically don't go far enough.

How did I destroy anything? As I said, I didn't change any existing behaviour, only added configurability. I'm sorry I caused more work for you.

comment:21 follow-ups: Changed 20 months ago by fbissey

There are a couple of instances that are glaring to me because

1) I provide my own minimal sage-env and anything that isn't useful has been thrown out. That includes GP_DATA_DIR and SINGULARPATH. Never used that before.

2) I don't include it in the default environment.

When working on env.py we basically reduced the use of os.environ to what was strictly necessary. So in the extra work I had on my plate

  • GAP_BINARY, you changed what to find to something different. gap is admittedly not a particularly well packaged software but using the gap in PATH was good for me.
  • MAXIMA_FAS, you actually introduce a more stringent check here. The issue is clearly on Gentoo side in some ways but it introduce the always problematic use of lib when on distro it could be lib64 (there is not a really good solution to that particular one so let it live). The Gentoo more specific issue is that the ecl folder is versioned (ecl-16.1.2 instead of ecl) the real problem with that is if we have wriggle room on the version of ecl (we don't at the moment).
  • SINGULARPATH well it existed in sage-env. Looking at singular's manual itself it is useful that such variable should defined, although considering that I never had an issue with singular 4 without defining it or overriding the default in the source code (like gentoo was doing in singular-3), it may not be that useful anymore and the documentation may be incorrect. Anyway I'd rather have a value from env.py because if SINGULARPATH is changed, no one may remember to apply the corresponding fix in singular.py.
  • GP_DATA_DIR is an interesting one because again it is only really used until now for the database_pari spkg and you have just extended its use. What is more interesting is that the old way and your new way are probably both broken in sage-on-gentoo but it has never been exposed as such. It may very well be that this setting is not necessary or that something more subtle needs doing at the level of the packaging of pari databases.

There are probably things where we'll have to agree to disagree and I am quite fine with that. I have been in such position with other sage-devs for 10 years after all.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 20 months ago by gh-timokau

Replying to fbissey:

There are a couple of instances that are glaring to me because

1) I provide my own minimal sage-env and anything that isn't useful has been thrown out. That includes GP_DATA_DIR and SINGULARPATH. Never used that before.

That's fair, GP_DATA_DIR and SINGULARPATH should've been done differently in this ticket. For what its worth what I do in the nix package is provide a wrapper around sage-env. So I source the original file and then add my own configuration on top.

2) I don't include it in the default environment.

When working on env.py we basically reduced the use of os.environ to what was strictly necessary. So in the extra work I had on my plate

  • GAP_BINARY, you changed what to find to something different. gap is admittedly not a particularly well packaged software but using the gap in PATH was good for me.

Note that before it didn't use the gap in PATH, but the gap in SAGE_LOCAL/bin/gap. That might coincidentally have been the one in PATH. I don't remember why the file is suddenly named gap.sh instead of gap.

  • MAXIMA_FAS, you actually introduce a more stringent check here. The issue is clearly on Gentoo side in some ways but it introduce the always problematic use of lib when on distro it could be lib64 (there is not a really good solution to that particular one so let it live). The Gentoo more specific issue is that the ecl folder is versioned (ecl-16.1.2 instead of ecl) the real problem with that is if we have wriggle room on the version of ecl (we don't at the moment).

That's why its configurable :)

  • SINGULARPATH well it existed in sage-env. Looking at singular's manual itself it is useful that such variable should defined, although considering that I never had an issue with singular 4 without defining it or overriding the default in the source code (like gentoo was doing in singular-3), it may not be that useful anymore and the documentation may be incorrect. Anyway I'd rather have a value from env.py because if SINGULARPATH is changed, no one may remember to apply the corresponding fix in singular.py.

I agree. However I think that it would be a better option for you to re-think your completely replaced sage-env. Maybe remove variables that don't actually need to be set upstream. Or even better fully replace the whole thing. The way it currently is, it is hard for me to know that I broke something on your modified install.

  • GP_DATA_DIR is an interesting one because again it is only really used until now for the database_pari spkg and you have just extended its use. What is more interesting is that the old way and your new way are probably both broken in sage-on-gentoo but it has never been exposed as such. It may very well be that this setting is not necessary or that something more subtle needs doing at the level of the packaging of pari databases.

Relevant: #26010

There are probably things where we'll have to agree to disagree and I am quite fine with that. I have been in such position with other sage-devs for 10 years after all.

Until now I don't disagree with anything you've said (except maybe completely replacing sage-env). I didn't realize that some of my changes were breaking. I should've requested feedback in sage-packaging. Sorry about that.

comment:23 follow-ups: Changed 20 months ago by fbissey

The gap issue is mainly because I deemed GAP_ROOT_DIR useless a long time ago. You are actually now pointing to a different gap executable. There is a "gap" startup script in SAGE_LOCAL/bin, which then starts gap with gap.sh found in SAGE_LOCAL/gap/latest/bin/. You basically switched from the first one to the second one by using GAP_ROOT_DIR. The stuff for which all that is used stinks to be honest and is not particularly useful to distro. May be I should add it to my "neutering" list.

My default position is that if I never run sage-env and import sage from a pure python shell it should work. And apart potentially from the issue of GP_DATA_DIR above, it just does.

Now from a distro point of view, definition of stuff like SINGULARPATH and GP_DATA_DIR should lay with their respective packages not sage or sage-env. Erik Bray is also in deep thought about how that kind of stuff should be implemented. So we can think of this things as a work in progress.

comment:24 in reply to: ↑ 23 ; follow-up: Changed 20 months ago by gh-timokau

Replying to fbissey:

The gap issue is mainly because I deemed GAP_ROOT_DIR useless a long time ago. You are actually now pointing to a different gap executable. There is a "gap" startup script in SAGE_LOCAL/bin, which then starts gap with gap.sh found in SAGE_LOCAL/gap/latest/bin/. You basically switched from the first one to the second one by using GAP_ROOT_DIR. The stuff for which all that is used stinks to be honest and is not particularly useful to distro. May be I should add it to my "neutering" list.

So basically just a symlink? At least the doctests still pass with the new executable. I don't know what you mean by neutering list, but it would be best to fix it upstream (either here or in gap) instead of just for Gentoo.

My default position is that if I never run sage-env and import sage from a pure python shell it should work. And apart potentially from the issue of GP_DATA_DIR above, it just does.

Yes, that would be perfect. If it works like that with the current setup however, thats mostly a coincidence. Your locations just happen to match up with the defaults in env.py, given a properly set SAGE_LOCAL I guess.

Now from a distro point of view, definition of stuff like SINGULARPATH and GP_DATA_DIR should lay with their respective packages not sage or sage-env. Erik Bray is also in deep thought about how that kind of stuff should be implemented. So we can think of this things as a work in progress.

Yes, in a better world singular and pari would find their own data.

comment:25 in reply to: ↑ 24 Changed 20 months ago by fbissey

Replying to gh-timokau:

Replying to fbissey:

The gap issue is mainly because I deemed GAP_ROOT_DIR useless a long time ago. You are actually now pointing to a different gap executable. There is a "gap" startup script in SAGE_LOCAL/bin, which then starts gap with gap.sh found in SAGE_LOCAL/gap/latest/bin/. You basically switched from the first one to the second one by using GAP_ROOT_DIR. The stuff for which all that is used stinks to be honest and is not particularly useful to distro. May be I should add it to my "neutering" list.

So basically just a symlink? At least the doctests still pass with the new executable. I don't know what you mean by neutering list, but it would be best to fix it upstream (either here or in gap) instead of just for Gentoo.

A shell script pointing to another shell script actually. "neutering" is mostly a reference to the fact that I remove the packaging abilities of sage to avoid accident. I only keep some minimum to avoid massive patching (go features). By extension anything that doesn't fit with a distro is "neutered". And don't worry i am working on fixing upstream but some of this stuff takes a long time to mature and quite a lot of advocacy.

comment:26 in reply to: ↑ 23 Changed 20 months ago by jdemeyer

Follow-up: #26025

comment:27 in reply to: ↑ 21 Changed 20 months ago by jdemeyer

Replying to fbissey:

  • GP_DATA_DIR is an interesting one because again it is only really used until now for the database_pari spkg

Just to clarify one thing: it's not only used by database_pari (which is going to be split in #26010). There are two PARI databases which are standard packages: pari_galdata and pari_seadata_small.

comment:28 in reply to: ↑ 22 ; follow-up: Changed 20 months ago by arojas

  • MAXIMA_FAS, you actually introduce a more stringent check here. The issue is clearly on Gentoo side in some ways but it introduce the always problematic use of lib when on distro it could be lib64 (there is not a really good solution to that particular one so let it live). The Gentoo more specific issue is that the ecl folder is versioned (ecl-16.1.2 instead of ecl) the real problem with that is if we have wriggle room on the version of ecl (we don't at the moment).

That's why its configurable :)

I just found out about this commit. This is also creating issues for Arch. Until now, ECLDIR wasn't used anywhere in Sage code proper, so we simply unset it and everything worked out of the box. With this commit, ECLDIR is required to be set. Since our ecl is installed to a versioned subdir of /usr/lib (which is the default upstream behavior, the /usr/lib/ecl symlink is Sage specific), we are forced to keep track of the ecl version in the Sage build script, and update and rebuild the package for every single ecl version bump.

What is making MAXIMA_FAS configurable trying to solve? maxima.fas is an ecl module, it must be installed in the ecl module dir. Is there an actual use case for installing it somewhere else?

I'm OK with the rest of the patch, but plase reconsider relying on ECLDIR in Sage code.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 20 months ago by gh-timokau

Replying to arojas:

I just found out about this commit. This is also creating issues for Arch. Until now, ECLDIR wasn't used anywhere in Sage code proper, so we simply unset it and everything worked out of the box. With this commit, ECLDIR is required to be set.

It only uses ECLDIR as a default for MAXIMA_FAS, so your issue is actually with MAXIMA_FAS right?

Since our ecl is installed to a versioned subdir of /usr/lib (which is the default upstream behavior, the /usr/lib/ecl symlink is Sage specific), we are forced to keep track of the ecl version in the Sage build script, and update and rebuild the package for every single ecl version bump.

Can't you just use a shell glob for the version, or are you using multiple maxima versions at the same time?

What is making MAXIMA_FAS configurable trying to solve? maxima.fas is an ecl module, it must be installed in the ecl module dir. Is there an actual use case for installing it somewhere else?

Well after this patch it doesnt need to be there anymore. The use case is that nix installs every package into its own prefix. Unfortunately ecl doesn't have an equivalent to PYTHONPATH.

I'm OK with the rest of the patch, but plase reconsider relying on ECLDIR in Sage code.

We could revert to the old behaviour in case MAXIMA_FAS and ECLDIR are both unset. Unfortunately I wont be able to implement that myself, because I dislocated my shoulder. Typing with one hand is no fun, let alone coding.

comment:30 in reply to: ↑ 29 Changed 20 months ago by arojas

Replying to gh-timokau:

Can't you just use a shell glob for the version, or are you using multiple maxima versions at the same time?

What do you mean by this? ecl doesn't accept glob in the path of 'require'

We could revert to the old behaviour in case MAXIMA_FAS and ECLDIR are both unset. Unfortunately I wont be able to implement that myself, because I dislocated my shoulder. Typing with one hand is no fun, let alone coding.

The problem is that 'unsetting' it now requires more patching, since you're also setting it in env.py. From my POW this is a step backwards in both

  • making sage work with unpatched dependencies (since it relies on the sage-specific /usr/lib/ecl path)
  • making sage more distribution-friendly (since it requires more patching to make it work with distro packages)

Anyway, I've opened #26100 with a proposed simple solution

comment:31 Changed 20 months ago by gh-timokau

I'm sorry for the inconvenience. The patch is a big improvement for *my* distribution, but I see how it makes things worse for some others.

comment:32 Changed 20 months ago by jdemeyer

More breakage: #26114

Note: See TracTickets for help on using tickets.