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

Fixes #2847 #2851

Closed
wants to merge 1 commit into from
Closed

Fixes #2847 #2851

wants to merge 1 commit into from

Conversation

SuperSandro2000
Copy link

No description provided.

@SuperSandro2000
Copy link
Author

Reverted the Unix changes.

@sampsyo
Copy link
Member

sampsyo commented Mar 22, 2018

Thanks! I like the idea to avoid exec altogether on Windows; this should probably make things more predictable.

If we're not using exec, however, maybe the right thing to use here would be subprocess, which is less error prone than system. If we do that, it seems like we could use shlex_split on both platforms.

The AppVeyor output points toward the tests that need some additional examination:
https://ci.appveyor.com/project/beetbox/beets/build/2415/job/w43io2yinu90h50b#L122

@SuperSandro2000
Copy link
Author

Sounds reasonable. Including shlex_split should fix the test with raise error and the other one expects exec to be called so I think it needs to be updated.

@SuperSandro2000
Copy link
Author

SuperSandro2000 commented Mar 22, 2018

Implemented subprocess. We need to call cmd.exe /c "start targets" as python is not aware of cmd-based command. I would be quite happy if you could help me with the tests. I know I need to change something here https://github.com/beetbox/beets/blob/master/test/test_config_command.py#L113 but I am not sure how to do it right.

Edit: I hope I changed the tests correctly.

@SuperSandro2000
Copy link
Author

I am not sure about the tests. Works but I think the behavior is different with os.exelp and subprocess.call.

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Thank you for working out the details here! I left one comment inline about dealing with shell argument weirdness.

base_cmd = 'start'
# python is not aware of cmd commands
# Start cmd.exe with start as argument
base_cmd = 'cmd /c \"start {}\"'
Copy link
Member

Choose a reason for hiding this comment

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

Here's the last thing we need a way around: using interpolation will cause unpredictable problems here when the filename contains special characters (including spaces). Do you know whether it's possible to get the same effect while using proper argument lists?

For example, what I would want to work would be:

subprocess.call('start', filename)

but, as you realized, that won't work because start is a cmd shell built-in. See this SO answer, for example.

Is there any clever way we can pass the filename argument as a proper shell argument without shell interpolation? As a last resort, we may need to escape the filename ourselves. 😕

Copy link
Author

Choose a reason for hiding this comment

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

subprocess.call(["cmd", "/c", "start", file])
I got this working

@jtpavlock
Copy link
Contributor

@SuperSandro2000 do you still intend to continue this?

@SuperSandro2000
Copy link
Author

@jtpavlock It was dead for 2.5 years not from my side.

@jtpavlock
Copy link
Contributor

Well if you're still interested in getting it merged, it would need at the very least a relevant change log entry/docs and maybe we can get @sampsyo to give it another look over.

@SuperSandro2000
Copy link
Author

@jtpavlock Replaced by #3665

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.

3 participants