#30563 closed enhancement (fixed)
Use configuration variable MAXIMA instead of hardcoding "maxima"
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  build: configure  Keywords:  
Cc:  thansen, mjo, fbissey  Merged in:  
Authors:  François Bissey  Reviewers:  Matthias Koeppe 
Report Upstream:  N/A  Work issues:  
Branch:  3da851f (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Continuation of #29038.
Upstreaming a generalized version of https://salsa.debian.org/scienceteam/sagemath//blob/master/debian/patches/d0maxima.patch
Change History (25)
comment:1 Changed 15 months ago by
 Description modified (diff)
comment:2 Changed 15 months ago by
 Cc mjo fbissey added
comment:3 followup: ↓ 4 Changed 15 months ago by
comment:4 in reply to: ↑ 3 Changed 15 months ago by
Replying to fbissey:
I am guessing that ideally we want to use the variable
MAXIMA
Yes, that's the idea
comment:5 followup: ↓ 6 Changed 15 months ago by
maxima.py
is already using MAXIMA
that leaves us with touching maxima_abstract.py
and a couple of things to help sageondebian pass its doctests out of the box.
Not sure how to proceed with bin/sage
as it is not reading that configuration file, nor should it.
comment:6 in reply to: ↑ 5 ; followup: ↓ 7 Changed 15 months ago by
Replying to fbissey:
Not sure how to proceed with
bin/sage
as it is not reading that configuration file, nor should it.
sageconfig MAXIMA
works if sage_conf
is installed, can fall back to maxima
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 15 months ago by
Replying to mkoeppe:
Replying to fbissey:
Not sure how to proceed with
bin/sage
as it is not reading that configuration file, nor should it.
sageconfig MAXIMA
works ifsage_conf
is installed, can fall back tomaxima
OK, that's another issue, I don't like how sageconfig is installed as a separate package but this is kind of orthogonal.
comment:8 in reply to: ↑ 7 Changed 15 months ago by
Replying to fbissey:
I don't like how sageconfig is installed as a separate package but this is kind of orthogonal.
Yes, that's orthogonal.
comment:9 Changed 15 months ago by
 Branch set to u/fbissey/ticket_30563
 Commit set to 38010138ed9f9c8d405b4e521526a749fc399f37
I shouldn't just leave things sitting on my hard drive.
There is a couple more things to do before calling it done.
New commits:
3801013  use the MAXIMA variable in maxima_abstract

comment:10 Changed 15 months ago by
 Commit changed from 38010138ed9f9c8d405b4e521526a749fc399f37 to 1ce58a777cb26f6f8e0a92a351904d51832da886
Branch pushed to git repo; I updated commit sha1. New commits:
1ce58a7  Relax some doctesting of string outputs for sageondebian

comment:11 Changed 15 months ago by
 Commit changed from 1ce58a777cb26f6f8e0a92a351904d51832da886 to 10bafa6f07531cabfb56adb1868da44c1cb0874b
Branch pushed to git repo; I updated commit sha1. New commits:
10bafa6  use sageconfig to figure maxima in bin/sage

comment:12 Changed 15 months ago by
@mkoeppe was the last commit the kind of things you had in mind for src/bin/sage
?
comment:13 Changed 15 months ago by
Yes, something like this. Probably needs stderr redirection though.
comment:14 Changed 15 months ago by
Most definitely, but after a night sleep I think I need to change the design slightly.
comment:15 Changed 15 months ago by
 Commit changed from 10bafa6f07531cabfb56adb1868da44c1cb0874b to 5cedc360a97378e27fe1e5eff6fe34205a1fbb72
Branch pushed to git repo; I updated commit sha1. New commits:
5cedc36  redirect error message so it doesn't look scary when things are missing (in distros for example)

comment:16 Changed 15 months ago by
 Status changed from new to needs_review
I was overthinking things. The current branch takes care of most of the stuff in the debian patch and should work nicely in most distros with minimal effort.
comment:17 Changed 15 months ago by
+ exec "$maxima_cmd" "$@"
I think $maxima_cmd
shouldn't be quoted here
comment:18 Changed 15 months ago by
Funny, I thought it should be quoted, because it could be a string with spaces in it. maxima l ecl
specifically. But I initially had put "$maxima_cmd $@"
which failed miserably when calling ./sage maxima
without any argument because of the space. So if it is safe for the case above, I am OK with removing the quotes.
comment:19 Changed 15 months ago by
But exec "maxima l ecl"
will fail
comment:20 Changed 15 months ago by
Indeed it does. And even if I remove the quotes there is an extra bit that needs to be dealt with. The l
is getting interpreted in the []
on line 609 so I need to do something more careful.
comment:21 Changed 15 months ago by
 Commit changed from 5cedc360a97378e27fe1e5eff6fe34205a1fbb72 to 3da851ff141e4eba5035ebcb994907b4902ec7da
Branch pushed to git repo; I updated commit sha1. New commits:
3da851f  Simplify the maxima_cmd setting logic and make it more robust. Also get rid of problematic quotes.

comment:22 Changed 15 months ago by
Turns out my overnight thought of redesign are not wasted. It is much more elegant and less problematic. Any objection to the default being maxima l ecl
? That means it works in sageongentoo with the right interpreter even if we don't ship sageconfig
. It should work with any maxima used by sage.
comment:23 Changed 15 months ago by
 Reviewers set to Matthias Koeppe
 Status changed from needs_review to positive_review
Looks good to me.
comment:24 Changed 14 months ago by
 Branch changed from u/fbissey/ticket_30563 to 3da851ff141e4eba5035ebcb994907b4902ec7da
 Resolution set to fixed
 Status changed from positive_review to closed
comment:25 Changed 14 months ago by
 Commit 3da851ff141e4eba5035ebcb994907b4902ec7da deleted
I missed a case in my testing of maxima l ecl
. Follow up at #30676.
I am guessing that ideally we want to use the variable
MAXIMA
rather than thosemaxima
andmaximasage
from the patch. It could be interesting as I could replacewith a variable assignment in sageongentoo.