Opened 15 months ago

Closed 14 months ago

Last modified 14 months ago

#30563 closed enhancement (fixed)

Use configuration variable MAXIMA instead of hardcoding "maxima"

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Change History (25)

comment:1 Changed 15 months ago by mkoeppe

  • Description modified (diff)

comment:2 Changed 15 months ago by mkoeppe

  • Cc mjo fbissey added

comment:3 follow-up: Changed 15 months ago by fbissey

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 15 months ago by mkoeppe

Replying to fbissey:

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

Yes, that's the idea

comment:5 follow-up: Changed 15 months ago by fbissey

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 ; follow-up: Changed 15 months ago by 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

comment:7 in reply to: ↑ 6 ; follow-up: Changed 15 months ago by fbissey

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 15 months ago by mkoeppe

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 15 months ago by fbissey

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

3801013use the MAXIMA variable in maxima_abstract

comment:10 Changed 15 months ago by git

  • Commit changed from 38010138ed9f9c8d405b4e521526a749fc399f37 to 1ce58a777cb26f6f8e0a92a351904d51832da886

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

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

comment:11 Changed 15 months ago by git

  • Commit changed from 1ce58a777cb26f6f8e0a92a351904d51832da886 to 10bafa6f07531cabfb56adb1868da44c1cb0874b

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

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

comment:12 Changed 15 months ago by fbissey

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

comment:13 Changed 15 months ago by mkoeppe

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

comment:14 Changed 15 months ago by fbissey

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

comment:15 Changed 15 months ago by git

  • Commit changed from 10bafa6f07531cabfb56adb1868da44c1cb0874b to 5cedc360a97378e27fe1e5eff6fe34205a1fbb72

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 15 months ago by fbissey

  • Authors set to François Bissey
  • 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 mkoeppe

+    exec "$maxima_cmd" "$@"

I think $maxima_cmd shouldn't be quoted here

comment:18 Changed 15 months ago by fbissey

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 mkoeppe

But exec "maxima -l ecl" will fail

comment:20 Changed 15 months ago by fbissey

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 git

  • Commit changed from 5cedc360a97378e27fe1e5eff6fe34205a1fbb72 to 3da851ff141e4eba5035ebcb994907b4902ec7da

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 15 months ago by fbissey

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 15 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

Looks good to me.

comment:24 Changed 14 months ago by vbraun

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

  • Commit 3da851ff141e4eba5035ebcb994907b4902ec7da deleted

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

Note: See TracTickets for help on using tickets.