Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#17685 closed defect (fixed)

Wrong shell test for "sage -n=..."

Reported by: tmonteil Owned by:
Priority: major Milestone: sage-6.5
Component: scripts Keywords:
Cc: vbraun, fbissey, rbeezer Merged in:
Authors: Thierry Monteil Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 633f690 (Commits) Commit:
Dependencies: Stopgaps:

Description

As reported on this ask question, the command sage <filename> is understood as sage -n=... command if <filename> contains -n. This is due to a wrong use of shell regex.

$ sage file-name.py
CRITICAL:root:unknown notebook: None
Error, notebook must be one of default, ipython, sagenb but got None

Change History (18)

comment:1 Changed 5 years ago by tmonteil

  • Branch set to u/tmonteil/wrong_shell_test_for__sage__n_____

comment:2 Changed 5 years ago by tmonteil

  • Commit set to 633f690b9d5c354529ae60b252360877a07e7c20
  • Status changed from new to needs_review

New commits:

633f690#17685 better regex for sage ^-n=.*

comment:3 Changed 5 years ago by tmonteil

  • Cc vbraun added

comment:4 follow-up: Changed 5 years ago by vdelecroix

Hello,

Just by curiosity, why did you remove the "?

Vincent

comment:5 Changed 5 years ago by fbissey

  • Cc fbissey added

comment:6 Changed 5 years ago by fbissey

I have been getting that an awful lot with doctesting of test/cmdline.py. I wonder if I keep getting temporary filename that trigger this. I will eagerly test this.

comment:7 Changed 5 years ago by fbissey

  • Status changed from needs_review to positive_review

Funky I have been complaining about that since sage 6.4 at least and no one could give me a lead on what was wrong. I give this a positive review.

comment:8 follow-up: Changed 5 years ago by vdelecroix

  • Authors set to Thierry Monteil
  • Reviewers set to François Bissey

author name + reviewer name!

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

Replying to vdelecroix:

author name + reviewer name!

Yes I should have filled my bits. I know why I always had the problem on my main machine. The host name is part of the temporary file name. My machine hostname is "qcd-nzi3", automatic fail.

I think you cannot have the " because it would have to be matched with the rest of the expression, I could be wrong about that one.

comment:10 Changed 5 years ago by kcrisman

Nice work! If you wanted to remove the =~ at the same time (see here) that would be swell, if no one would object to it (improves portability, I would think).

comment:11 Changed 5 years ago by vbraun

  • Branch changed from u/tmonteil/wrong_shell_test_for__sage__n_____ to 633f690b9d5c354529ae60b252360877a07e7c20
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 5 years ago by rbeezer

  • Cc rbeezer added
  • Commit 633f690b9d5c354529ae60b252360877a07e7c20 deleted

comment:13 Changed 5 years ago by rbeezer

I added myself as a cc, and the changes say I deleted a commit? I hope I didn't make a mess here.

comment:14 Changed 5 years ago by fbissey

I don't think you deleted anything but now that it is merged it moved to a branch - I think. I won't try to comment on a closed ticket to check the behavior though.

comment:15 Changed 5 years ago by kcrisman

This has happened to me often enough that I am pretty sure fbissey is right - the next comment even after a push sometimes makes those messages appear (also even on github).

comment:16 in reply to: ↑ 4 Changed 5 years ago by tmonteil

Replying to vdelecroix:

Just by curiosity, why did you remove the "?

If i put a global quote "^-n=.*", then everything is escaped so ^ and .* will not be recognized as special characters (it will recognize strings containing the substring ^-n=.*). So i would have to do ^"-n=".* to only escape -n=. Now, looking at man 7 regex, it seems that = is special only if surrounded by some bracket, and - is special only for intervals like A-Z. So, since it is not the case, we should be safe without quotes (though i usually like to put lot of quotes in bash, just to be sure), hence i thought "if something goes wrong with that apparently less safe way, then it will be good because then we could help fixing bash."

Replying to kcrisman:

Nice work! If you wanted to remove the =~ at the same time (see here) that would be swell, if no one would object to it (improves portability, I would think).

It seems that i am a bit late, i did not think the ticket will be merged so fast. Actually, even [[ is a bashism, not only =~, so it is indeed a good proposition to get rid of this construction. See #17699 for a follow up.

comment:17 Changed 5 years ago by jhpalmieri

The issue isn't really "bashisms": the file starts with #!/usr/bin/env bash, so we're using bash. The issue is with compatibility with ancient versions of bash (pre version 3, I guess) which don't include things like =~.

comment:18 Changed 5 years ago by kcrisman

John P. is correct. That said, there probably are plenty of places we could increase portability - see all the Solaris and also other ports where this occasionally caused problems. Even Cygwin where dash is sometimes the only thing available...

Note: See TracTickets for help on using tickets.