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

Fix #13084 #13093

Merged
merged 9 commits into from
Aug 20, 2021
Merged

Fix #13084 #13093

merged 9 commits into from
Aug 20, 2021

Conversation

madbird1304
Copy link
Contributor

Fix for issue caused by using shlex to quote executable path.
Sorry, I've introduced this bug.
To fix this, let's replace shlex.quote with subprocess.list2cmd(). It

  • uses " to quote path,
  • performs quoting only if necessary

@MrMino , could you take a look please?
image

Copy link
Member

@MrMino MrMino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh... I didn't wanted list2cmdline cause it's MSC specific but it turns out that shlex.quote also isn't "platform agnostic". It didn't occur to me that single quotes would have different meaning to double quotes - I should've tested it on Windows.

One other argument against using list2cmdline: it isn't really "public" in the same sense other methods of subprocess are. It's an implementation detail, not a "public API".

Furthermore, I feel like introducing MSC shell semantics on all other platforms will introduce subtle bugs and edge cases. A simple .replace("'", "\"") also isn't enough, since virtualenv paths can contain " on UNIX-es.

At this point I'm questioning whether this should even use shell.system. I should probably look into creating a argv-style API for shell commands in magics. But let's fix this first. Any changes to the shell.system API would probably need to go into 8.0, and we need to get the fix into 7.27.

I don't see any other option than using something like if sys.platform == "win32": "\"" + s + "\""; else: shlex.quote(...).

@MrMino MrMino changed the title Fix for https://github.com/ipython/ipython/issues/13084 Fix #13084 Aug 16, 2021
@MrMino MrMino self-assigned this Aug 16, 2021
@MrMino MrMino added this to the 7.27 milestone Aug 16, 2021
@madbird1304
Copy link
Contributor Author

@MrMino , you said:

At this point I'm questioning whether this should even use shell.system.

Then let's just get rid of it?
image

@MrMino
Copy link
Member

MrMino commented Aug 17, 2021

We cannot get rid of or substantially change shell.system until at least 8.0, and even then I'd rather not add more to an already bulky major release. The fix has to go into the next 7.x.

Coming up with something that is a valid replacement for it isn't trivial either.

Unless there's another option, I'd rather bodge it in with a sys.platform branch.

Copy link
Member

@MrMino MrMino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly what I had in mind :). Thanks!

If you add a whats-new entry and fix the linter errors, I'll merge.

Edit: @madbird1304 since I'm on vacation next week, and I don't want to postpone it till the next release, I went ahead and fixed the lint for you. Hope you don't mind.

@MrMino MrMino merged commit 96617c6 into ipython:master Aug 20, 2021
meeseeksmachine pushed a commit to meeseeksmachine/ipython that referenced this pull request Aug 20, 2021
@MrMino
Copy link
Member

MrMino commented Aug 20, 2021

Thanks @madbird1304!

@madbird1304
Copy link
Contributor Author

Sorry for late reply. I was busy last week, so thanks for finishing that without me.

@madbird1304 madbird1304 deleted the madbird1304/issue#13084-fix branch August 22, 2021 21:56
Carreau added a commit that referenced this pull request Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows %pip magic broken for 7.26 release due to shlex.quote pathing issue
2 participants