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

quoting fix #1115

Merged
merged 6 commits into from
Aug 17, 2021
Merged

quoting fix #1115

merged 6 commits into from
Aug 17, 2021

Conversation

nerdvegas
Copy link
Contributor

@nerdvegas nerdvegas commented Aug 3, 2021

Fixes #1114

Note that Windows test is now failing and that is intended, there's a preexisting bug there that we need to fix.

Related: #1111

@davidlatwe
Copy link
Contributor

Continuing #1116 (comment) here. :)

In #1116, I changed the test to match result with shlex joined input, and it passed in Windows but actually failed in other OS.

import subprocess
from shlex import join  # new in Python 3.8, just for quick testing

>>> subprocess.check_output(join(['echo', 'hey you']), shell=True, text=True).strip()
# Windows CMD
"'hey you'"
# In my WSL (Ubuntu)
'hey you'

# however..
>>> join(['echo', 'hey you'])
"echo 'hey you'"  # same result for both platform

So maybe we should match the result with actual shell output plus shlex joined input ? Like

def popen(args, shell=False):
    process = subprocess.Popen(
        args, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
        stderr=subprocess.PIPE, universal_newlines=True, shell=shell
    )
    sh_out = process.communicate()
    return sh_out[0].strip()

a = popen([rez_env_exec, "--", "echo", txt])
b = popen(shlex_join(["echo", txt]), shell=True)

self.assertEqual(a, b)

Kind of stupid but seems fair ?

Just for proving the concept, I have updated my branch and CI testing passed in both Ubuntu and Windows.

@nerdvegas
Copy link
Contributor Author

I dunno, that seems oddly circular... all we're really doing at that point is verifying that rez-env is running the given command in a shell. Also isn't there no guarantee that the current shell (ie whatr raw subprocess will use if shell=True) will match the shell rez is using?

I'm starting to think we simply need a cmd caveat here.

@davidlatwe
Copy link
Contributor

Yeah.. :(
Or, how about just validate the test cases with hard coded results ? Like..

    @per_available_shell()
    @install_dependent()
    def test_rez_env_output(self):

        cases = {
            "hey": {},
            "hey you": {"cmd": '"hey you"'},
            ...
        }

        def _test(txt):
            ...
            expect = cases[txt].get(config.default_shell, txt)
            self.assertEqual(sh_out[0].strip(), expect)

        _test("hey")  # simple case
        _test("hey you")  # with a space

@nerdvegas nerdvegas merged commit 6a1d8e6 into master Aug 17, 2021
@bpabel bpabel deleted the issue_1114-command-quoting branch January 19, 2023 20:34
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.

bug in command quoting
2 participants