#26100 closed enhancement (fixed)

Always try to load maxima.fas from the standard ecl module dir

Reported by: arojas Owned by:
Priority: major Milestone: sage-8.4
Component: distribution Keywords:
Cc: saraedum, gh-timokau, fbissey, jdemeyer Merged in:
Authors: Antonio Rojas Reviewers: Timo Kaufmann
Report Upstream: N/A Work issues:
Branch: 5b9d903 (Commits) Commit: 5b9d9033ec64d676b9813653ec7d4896cfb8f378
Dependencies: Stopgaps:

Description (last modified by arojas)

In #25309 the path to the maxima ecl module was made configurable. However, the default path /usr/lib/ecl/maxima.fas is sage-specific (upstream ecl installs modules to the versioned path /usr/lib/ecl-${eclver}). This forces distributions who ship unmodified ecl to patch sage to point it to the right path. Even worse, the path depends on the ecl version so the patch would need to be updated for every ecl upgrade, creating additional burden.

This patch makes makes MAXIMA_FAS unset by default. For sage-the-distribution the behavior should be identical to 8.3. For distros that ship a vanilla ecl (ie. with versioned module dir) it should work out of the box as it did in 8.3. Distros that install maxima.fas to a non-standard location can still customize it.

Change History (23)

comment:1 Changed 21 months ago by arojas

  • Branch set to u/arojas/always_try_to_load_maxima_fas_from_the_standard_ecl_module_dir

comment:2 Changed 21 months ago by arojas

  • Commit set to db33b02267fec6c5aa9c7dd63986c5653ca9dd55
  • Component changed from PLEASE CHANGE to distribution
  • Description modified (diff)
  • Type changed from PLEASE CHANGE to enhancement

New commits:

db33b02Try loading maxima.fas from the standard ecl module dir if the path set in MAXIMA_FAS doesn't exist

comment:3 Changed 21 months ago by arojas

  • Cc saraedum gh-timokau fbissey jdemeyer added

comment:4 Changed 21 months ago by arojas

  • Status changed from new to needs_review

comment:5 Changed 21 months ago by gh-timokau

This solution is essentially what I meant in #25309. I just don't like that it silently fails if I explicitly provides MAXIMA_FAS. I'd prefer it if ECLDIR would just default to None in env.py and the default is only used when it is None (not explicitly overwritten).

comment:6 Changed 21 months ago by arojas

Setting ECLDIR and MAXIMA_FAS to None by default in env.py and replacing the 'try:' with an 'if "MAXIMA_FAS" in os.environ:' would also work for me. Let's see if someone else has an opinion on this.

comment:7 follow-up: Changed 21 months ago by gh-timokau

MAXIMA_FAS should probably still default relative to ecl dir if it is set. Or is that the default behaviour anyways?

comment:8 Changed 21 months ago by git

  • Commit changed from db33b02267fec6c5aa9c7dd63986c5653ca9dd55 to c54ebaeccbeff854ac280bcf1a963e2b150871f7

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

c54ebaeDon't set ECLDIR and MAXIMA_FAS by default in env.py

comment:9 in reply to: ↑ 7 Changed 21 months ago by arojas

Replying to gh-timokau:

MAXIMA_FAS should probably still default relative to ecl dir if it is set. Or is that the default behaviour anyways?

That's the default behavior, yes. Since ECLDIR is used nowhere else, there should be no need to set it, right? You can simply define MAXIMA_FAS directly.

comment:10 Changed 21 months ago by arojas

  • Description modified (diff)

comment:11 Changed 21 months ago by gh-timokau

Yes in that case we could just get rid of ECLDIR.

comment:12 Changed 21 months ago by gh-timokau

... which I see you already did.

comment:13 follow-up: Changed 21 months ago by gh-timokau

Minor style nitpick: I very much prefer if MAXIMA_FAS is not None over if MAXIMA_FAS. The second one looks like MAXIMA_FAS is a boolean. Not sure if sage has style guides for that.

comment:14 in reply to: ↑ 13 Changed 21 months ago by nbruin

Replying to gh-timokau:

Minor style nitpick: I very much prefer if MAXIMA_FAS is not None over if MAXIMA_FAS. The second one looks like MAXIMA_FAS is a boolean. Not sure if sage has style guides for that.

In fact, ... is not None is the appropriate test if None is used as a sentinel value. The empty string "" would also be "False", and I imagine you might want to treat it differently from the value not being present. There are some wilder scenarios where it matters, e.g. if someone makes a string type that always tests False, but that is pretty exotic.

comment:15 follow-up: Changed 21 months ago by gh-timokau

For what its worth I still have to set ECLDIR for the nix package. I'm not sure why ECL needs that or what the default is when it is not set. Have you tested sage without a system ECL installed? Maybe its accidentally using that when removing ECLDIR.

comment:16 in reply to: ↑ 15 Changed 21 months ago by arojas

Replying to gh-timokau:

For what its worth I still have to set ECLDIR for the nix package. I'm not sure why ECL needs that or what the default is when it is not set. Have you tested sage without a system ECL installed? Maybe its accidentally using that when removing ECLDIR.

From ecl's configure script:

AC_ARG_VAR([ecldir], [the directory where *.fas files are installed]) test -z "${ecldir}" && ecldir="${libdir}/ecl-${PACKAGE_VERSION}"

Sage works fine without a system ecl and without setting ECLDIR (even after removing it from sage-env). From https://trac.sagemath.org/ticket/7186 setting it was added to fix issues after moving the sage tree - I wonder if that still makes sense now that moving the tree is no longer supported

comment:17 Changed 21 months ago by git

  • Commit changed from c54ebaeccbeff854ac280bcf1a963e2b150871f7 to 5b9d9033ec64d676b9813653ec7d4896cfb8f378

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

5b9d903Use 'if MAXIMA_FAS is not None'

comment:18 follow-up: Changed 21 months ago by gh-timokau

  • Reviewers set to Timo Kaufmann
  • Status changed from needs_review to positive_review

Out of interest: Do you happen to know how ecl knows sage's lib_dir?

comment:19 in reply to: ↑ 18 Changed 21 months ago by arojas

Replying to gh-timokau:

Out of interest: Do you happen to know how ecl knows sage's lib_dir?

sdh_configure does that

https://github.com/sagemath/sage/blob/master/src/bin/sage-dist-helpers#L150

comment:20 Changed 21 months ago by gh-timokau

Hm weird, we set the correct --prefix for ecl and the lib dir is under <prefix>/lib as usual, so I wonder why ecl doesn't figure it out by itself in our case. Something to investigate in the future. Thanks :)

comment:21 Changed 21 months ago by vbraun

  • Status changed from positive_review to needs_work

Author name is missing...

comment:22 Changed 21 months ago by arojas

  • Authors set to Antonio Rojas
  • Status changed from needs_work to positive_review

comment:23 Changed 21 months ago by vbraun

  • Branch changed from u/arojas/always_try_to_load_maxima_fas_from_the_standard_ecl_module_dir to 5b9d9033ec64d676b9813653ec7d4896cfb8f378
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.