Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#30563 closed enhancement (fixed)

Use configuration variable MAXIMA instead of hardcoding "maxima"

Reported by: Matthias Köppe Owned by:
Priority: major Milestone: sage-9.2
Component: build: configure Keywords:
Cc: Tobias Hansen, Michael Orlitzky, François Bissey Merged in:
Authors: François Bissey Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 3da851f (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Change History (25)

comment:1 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:2 Changed 2 years ago by Matthias Köppe

Cc: Michael Orlitzky François Bissey added

comment:3 Changed 2 years ago by François Bissey

I am guessing that ideally we want to use the variable MAXIMA rather than those maxima and maxima-sage from the patch. It could be interesting as I could replace

	# run maxima with ecl
	sed -i "s:'maxima :'maxima -l ecl :g" \
		sage/interfaces/maxima.py \
		sage/interfaces/maxima_abstract.py

with a variable assignment in sage-on-gentoo.

comment:4 in reply to:  3 Changed 2 years ago by Matthias Köppe

Replying to fbissey:

I am guessing that ideally we want to use the variable MAXIMA

Yes, that's the idea

comment:5 Changed 2 years ago by François Bissey

maxima.py is already using MAXIMA that leaves us with touching maxima_abstract.py and a couple of things to help sage-on-debian 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 ; Changed 2 years ago by Matthias Köppe

Replying to fbissey:

Not sure how to proceed with bin/sage as it is not reading that configuration file, nor should it.

sage-config MAXIMA works if sage_conf is installed, can fall back to maxima

comment:7 in reply to:  6 ; Changed 2 years ago by François Bissey

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.

sage-config MAXIMA works if sage_conf is installed, can fall back to maxima

OK, that's another issue, I don't like how sage-config is installed as a separate package but this is kind of orthogonal.

comment:8 in reply to:  7 Changed 2 years ago by Matthias Köppe

Replying to fbissey:

I don't like how sage-config is installed as a separate package but this is kind of orthogonal.

Yes, that's orthogonal.

comment:9 Changed 2 years ago by François Bissey

Branch: u/fbissey/ticket_30563
Commit: 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:

3801013use the MAXIMA variable in maxima_abstract

comment:10 Changed 2 years ago by git

Commit: 38010138ed9f9c8d405b4e521526a749fc399f371ce58a777cb26f6f8e0a92a351904d51832da886

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

1ce58a7Relax some doctesting of string outputs for sage-on-debian

comment:11 Changed 2 years ago by git

Commit: 1ce58a777cb26f6f8e0a92a351904d51832da88610bafa6f07531cabfb56adb1868da44c1cb0874b

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

10bafa6use sage-config to figure maxima in bin/sage

comment:12 Changed 2 years ago by François Bissey

@mkoeppe was the last commit the kind of things you had in mind for src/bin/sage?

comment:13 Changed 2 years ago by Matthias Köppe

Yes, something like this. Probably needs stderr redirection though.

comment:14 Changed 2 years ago by François Bissey

Most definitely, but after a night sleep I think I need to change the design slightly.

comment:15 Changed 2 years ago by git

Commit: 10bafa6f07531cabfb56adb1868da44c1cb0874b5cedc360a97378e27fe1e5eff6fe34205a1fbb72

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

5cedc36redirect error message so it doesn't look scary when things are missing (in distros for example)

comment:16 Changed 2 years ago by François Bissey

Authors: François Bissey
Status: newneeds_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 2 years ago by Matthias Köppe

+    exec "$maxima_cmd" "$@"

I think $maxima_cmd shouldn't be quoted here

comment:18 Changed 2 years ago by François Bissey

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 2 years ago by Matthias Köppe

But exec "maxima -l ecl" will fail

comment:20 Changed 2 years ago by François Bissey

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

Commit: 5cedc360a97378e27fe1e5eff6fe34205a1fbb723da851ff141e4eba5035ebcb994907b4902ec7da

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

3da851fSimplify the maxima_cmd setting logic and make it more robust. Also get rid of problematic quotes.

comment:22 Changed 2 years ago by François Bissey

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 sage-on-gentoo with the right interpreter even if we don't ship sage-config. It should work with any maxima used by sage.

comment:23 Changed 2 years ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

Looks good to me.

comment:24 Changed 2 years ago by Volker Braun

Branch: u/fbissey/ticket_305633da851ff141e4eba5035ebcb994907b4902ec7da
Resolution: fixed
Status: positive_reviewclosed

comment:25 Changed 2 years ago by François Bissey

Commit: 3da851ff141e4eba5035ebcb994907b4902ec7da

I missed a case in my testing of maxima -l ecl. Follow up at #30676.

Note: See TracTickets for help on using tickets.