Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use configuration variable MAXIMA instead of hardcoding "maxima" #30563

Closed
mkoeppe opened this issue Sep 12, 2020 · 33 comments
Closed

Use configuration variable MAXIMA instead of hardcoding "maxima" #30563

mkoeppe opened this issue Sep 12, 2020 · 33 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Sep 12, 2020

Continuation of #29038.

Upstreaming a generalized version of
https://salsa.debian.org/science-team/sagemath/-/blob/master/debian/patches/d0-maxima.patch

CC: @tobihan @orlitzky @kiwifb

Component: build: configure

Author: François Bissey

Branch: 3da851f

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/30563

@mkoeppe mkoeppe added this to the sage-9.2 milestone Sep 12, 2020
@mkoeppe

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Sep 14, 2020

comment:3

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 14, 2020

comment:4

Replying to @kiwifb:

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

Yes, that's the idea

@kiwifb
Copy link
Member

kiwifb commented Sep 14, 2020

comment:5

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 14, 2020

comment:6

Replying to @kiwifb:

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

@kiwifb
Copy link
Member

kiwifb commented Sep 14, 2020

comment:7

Replying to @mkoeppe:

Replying to @kiwifb:

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 14, 2020

comment:8

Replying to @kiwifb:

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

Yes, that's orthogonal.

@kiwifb
Copy link
Member

kiwifb commented Sep 15, 2020

Commit: 3801013

@kiwifb
Copy link
Member

kiwifb commented Sep 15, 2020

Branch: u/fbissey/ticket_30563

@kiwifb
Copy link
Member

kiwifb commented Sep 15, 2020

comment:9

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2020

Changed commit from 3801013 to 1ce58a7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2020

Changed commit from 1ce58a7 to 10bafa6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2020

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

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

@kiwifb
Copy link
Member

kiwifb commented Sep 15, 2020

comment:12

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 15, 2020

comment:13

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

@kiwifb
Copy link
Member

kiwifb commented Sep 15, 2020

comment:14

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2020

Changed commit from 10bafa6 to 5cedc36

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 15, 2020

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)

@kiwifb
Copy link
Member

kiwifb commented Sep 15, 2020

Author: François Bissey

@kiwifb
Copy link
Member

kiwifb commented Sep 15, 2020

comment:16

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 16, 2020

comment:17
+    exec "$maxima_cmd" "$@"

I think $maxima_cmd shouldn't be quoted here

@kiwifb
Copy link
Member

kiwifb commented Sep 16, 2020

comment:18

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 16, 2020

comment:19

But exec "maxima -l ecl" will fail

@kiwifb
Copy link
Member

kiwifb commented Sep 16, 2020

comment:20

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2020

Changed commit from 5cedc36 to 3da851f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 16, 2020

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.

@kiwifb
Copy link
Member

kiwifb commented Sep 16, 2020

comment:22

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 16, 2020

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 16, 2020

comment:23

Looks good to me.

@vbraun
Copy link
Member

vbraun commented Sep 27, 2020

Changed branch from u/fbissey/ticket_30563 to 3da851f

@kiwifb
Copy link
Member

kiwifb commented Sep 28, 2020

Changed commit from 3da851f to none

@kiwifb
Copy link
Member

kiwifb commented Sep 28, 2020

comment:25

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants