Opened 2 years ago

Closed 2 years ago

#30676 closed defect (fixed)

Follow up to #30563 too many quotes in maxima.py

Reported by: fbissey Owned by:
Priority: critical Milestone: sage-9.2
Component: interfaces Keywords:
Cc: strogdon Merged in:
Authors: François Bissey Reviewers: Steven Trogdon, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: c330344 (Commits, GitHub, GitLab) Commit: c3303445614cb8ebe30a23356b3083f85ce8df58
Dependencies: Stopgaps:

Status badges

Description

In #30563 one particular setting of MAXIMA used in sage-on-gentoo (maximax -l ecl) was extensively tested when applied to bin/sage. It unfortunately hadn't been so thoroughly tested in interfaces/maxima.py.

The __init__ method in maxima.py quote the variable MAXIMA when building the command for pexpect

                        command = '"{0}" -p "{1}"'.format(MAXIMA, STARTUP),

this leads the whole expression maxima -l ecl, with spaces, to be considered a command by pexpect. Removing the double quotes fix the problem and doesn't introduce any issue with more conventional values of MAXIMA.

Change History (19)

comment:1 Changed 2 years ago by fbissey

Authors: François Bissey
Branch: u/fbissey/track_30676
Cc: strogdon added
Commit: 8bef77e2b6685dbc5da0be0251e1f47e3473db86
Status: newneeds_review

@strogdon, you may have seen that issue.


New commits:

8bef77eRemove superfluous, and dangerous, double quotes.

comment:2 Changed 2 years ago by mkoeppe

STARTUP should probably be quoted in case the directory contains whitespace.

comment:3 Changed 2 years ago by fbissey

OK, I guess someone would have complained already if putting double quote there broke things.

I'll add that in fact sage-on-broken was potentially broken if you had maxima with sbcl and ecl installed - from the day interfaces/maxima.py started to use MAXIMA. So those improvements were really needed.

comment:4 Changed 2 years ago by fbissey

Hum, then it may break things if MAXIMA is a path with spaces in it. A situation which could be common on windows.

comment:5 Changed 2 years ago by fbissey

#30563 would already cause trouble if MAXIMA is a path with spaces in it because I didn't double quote in that ticket either. Which may mean no bots has that kind of configuration.

comment:6 Changed 2 years ago by git

Commit: 8bef77e2b6685dbc5da0be0251e1f47e3473db86a6f2b0c7e3b2985586b8fe9f0b84fb7cfcfb3849

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a6f2b0cUnqute MAXIMA so that "maxima -l ecl" is not considered one single command by pexpect

comment:7 Changed 2 years ago by fbissey

If there are complaints I have a plan B.

comment:8 Changed 2 years ago by mkoeppe

Well, since MAXIMA is used as the beginning of a shell command line (this could be documented...), one would be able to use shell quoting within the value of this variable.

comment:9 in reply to:  8 Changed 2 years ago by fbissey

Replying to mkoeppe:

Well, since MAXIMA is used as the beginning of a shell command line (this could be documented...), one would be able to use shell quoting within the value of this variable.

Can you give a concrete example of your line of thought there? I am not completely sure of what your idea is and how to put it in practice.

comment:10 Changed 2 years ago by mkoeppe

If maxima lies in /path with spaces/, then MAXIMA could be set to '"/path with spaces/maxima" -l ecl' and that would work with the current version on the branch.

Unrelated, but perhaps an improvement:

- command = '{0} -p "{1}"'.format(MAXIMA, STARTUP)
+ command = '{0} -p {1}'.format(MAXIMA, shlex.quote(STARTUP))

https://docs.python.org/3/library/shlex.html

Last edited 2 years ago by mkoeppe (previous) (diff)

comment:11 Changed 2 years ago by fbissey

Thanks for the explanation, that was helpful.

Using stuff like shlex more seems like a good idea to me and quote is available from 3.3 so it is ok to use. I'll redo the branch shortly.

comment:12 Changed 2 years ago by git

Commit: a6f2b0c7e3b2985586b8fe9f0b84fb7cfcfb384948fde5407444a2203fbcda91a6c2ba39744b183a

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

48fde54Unquote MAXIMA to allow composite command and switch to shlex.quote for STARTUP.

comment:13 Changed 2 years ago by strogdon

Does shlex have to be imported somewhere? interfaces/maxima.py fails here on s-o-g unless I explicitly import shlex.

comment:14 Changed 2 years ago by fbissey

I'll confess that I thought it would be OK without import and it was foolish. I'll fix that.

comment:15 Changed 2 years ago by git

Commit: 48fde5407444a2203fbcda91a6c2ba39744b183ac3303445614cb8ebe30a23356b3083f85ce8df58

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

c330344Import shlex as it is needed

comment:16 Changed 2 years ago by fbissey

Done.

comment:17 Changed 2 years ago by strogdon

Reviewers: Steven Trogdon
Status: needs_reviewpositive_review

This works here.

@mkoeppe please add yourself if you see fit as I would never have thought of using shlex.

comment:18 Changed 2 years ago by mkoeppe

Reviewers: Steven TrogdonSteven Trogdon, Matthias Koeppe

comment:19 Changed 2 years ago by vbraun

Branch: u/fbissey/track_30676c3303445614cb8ebe30a23356b3083f85ce8df58
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.