search

AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
948 stars 336 forks source link

Quotation marks issues on Windows. #691

Closed KelSolaar closed 5 years ago

KelSolaar commented 5 years ago

Hi,

I'm testing 2.40.1 to upgrade from 2.16.0 and I'm having issues with quotation marks on Windows.

Specifically:

C:\Users\thomas>rez-env substance_painter -- "Substance Painter"
'Substance' is not recognized as an internal or external command,
operable program or batch file.

It could be related to #664 but I'm not sure yet, I will try to take a look.

Cheers,

Thomas

nerdvegas commented 5 years ago

Hi Thomas,

I wonder also if it's related to https://github.com/nerdvegas/rez/releases/tag/2.35.0. Can you test that and the version before it? (https://github.com/nerdvegas/rez/releases/tag/2.34.0 ).

The changes in 2.35.0 do potentially affect how the command is escaped in cmd. If that is the case, it's certainly worth looking at the merged PR thread. Please let me know how you go!

Thanks, A

On Tue, Aug 13, 2019 at 9:46 AM Thomas Mansencal notifications@github.com wrote:

Hi,

I'm testing 2.40.1 to upgrade from 2.16.0 and I'm having issues with quotation marks on Windows.

Specifically:

C:\Users\thomas>rez-env substance_painter -- "Substance Painter" 'Substance' is not recognized as an internal or external command, operable program or batch file.

It could be related to #664 https://github.com/nerdvegas/rez/issues/664 but I'm not sure yet, I will try to take a look.

Cheers,

Thomas

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nerdvegas/rez/issues/691?email_source=notifications&email_token=AAMOUSTSUUIIN674MR2B5GTQEHY5PA5CNFSM4ILF4AF2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HE2VB4Q, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMOUSWBEA274HJS6XTKTV3QEHY5PANCNFSM4ILF4AFQ .

KelSolaar commented 5 years ago

Hi @nerdvegas,

Confirming that 2.35.0 introduced the issue, 2.34.0 is working as expected! I will look at what the PR changed.

Cheers,

KelSolaar commented 5 years ago

So quickly comparing the generated rez-shell.bat and the command call:

Rez 2.34.0

rez-shell.bat

set REZ_ENV_PROMPT=%REZ_ENV_PROMPT%$G
call c:\users\thomas\appdata\local\temp\rez_context_azh6ou\context.bat
set REZ_STORED_PROMPT=$P$G
set PROMPT=%REZ_ENV_PROMPT% $P$G
"Substance Painter"
exit %errorlevel%

Command Call

['c:\\windows\\system32\\cmd.exe', '/Q', '/C', 'call c:\\users\\thomas\\appdata\\local\\temp\\rez_context_azh6ou\\rez-shell.bat']

Rez 2.35.0

rez-shell.bat

set REZ_ENV_PROMPT=%REZ_ENV_PROMPT%$G
call c:\users\thomas\appdata\local\temp\rez_context_mjdt6d\context.bat
set REZ_STORED_PROMPT=$P$G
set PROMPT=%REZ_ENV_PROMPT% $P$G

Command Call

['c:\\windows\\system32\\cmd.exe', '/Q', '/C', 'call c:\\users\\thomas\\appdata\\local\\temp\\rez_context_mjdt6d\\rez-shell.bat', '& Substance Painter']

The double-quotes are effectively gone but to make matter worse, tweaking the code so that the command call is as follows:

['c:\\windows\\system32\\cmd.exe', '/Q', '/C', 'call c:\\users\\thomas\\appdata\\local\\temp\\rez_context_beyump\\rez-shell.bat', '& "Substance Painter"']

still errors out:

'\"Substance Painter\"' is not recognized as an internal or external command,
operable program or batch file.

Haven't followed exactly the PR discussions, I'm wondering what was the rationale for this separation compared to having everything in the .bat file?

KelSolaar commented 5 years ago

@mottosso : I suppose this current issue was introduced with #627, any idea of how to circumvent that?

nerdvegas commented 5 years ago

Best person to speak to would be @mottosso. If that release has caused problems that don't appear to be solvable, then it can be reverted.

Here is the first PR that you may have missed (it wasn't merged): https://github.com/nerdvegas/rez/pull/626 And there is the replacement PR that was merged into that release: https://github.com/nerdvegas/rez/pull/627

Would be great to get your input as I don't do the Windows stuff.

Thanks Thomas, A

On Tue, Aug 13, 2019 at 1:58 PM Thomas Mansencal notifications@github.com wrote:

So quickly comparing the generated rez-shell.bat and the command call: Rez 2.34.0

rez-shell.bat

set REZ_ENV_PROMPT=%REZ_ENV_PROMPT%$G call c:\users\thomas\appdata\local\temp\rez_context_azh6ou\context.batset REZ_STORED_PROMPT=$P$Gset PROMPT=%REZ_ENV_PROMPT% $P$G"Substance Painter"exit %errorlevel%

Command Call

['c:\windows\system32\cmd.exe', '/Q', '/C', 'call c:\users\thomas\appdata\local\temp\rez_context_azh6ou\rez-shell.bat']

Rez 2.35.0

rez-shell.bat

set REZ_ENV_PROMPT=%REZ_ENV_PROMPT%$G call c:\users\thomas\appdata\local\temp\rez_context_mjdt6d\context.batset REZ_STORED_PROMPT=$P$Gset PROMPT=%REZ_ENV_PROMPT% $P$G

Command Call

['c:\windows\system32\cmd.exe', '/Q', '/C', 'call c:\users\thomas\appdata\local\temp\rez_context_mjdt6d\rez-shell.bat', '& Substance Painter']

The double-quotes are effectively gone but to make matter worse, tweaking the code so that the command call is as follows:

['c:\windows\system32\cmd.exe', '/Q', '/C', 'call c:\users\thomas\appdata\local\temp\rez_context_beyump\rez-shell.bat', '& "Substance Painter"']

still errors out:

'\"Substance Painter\"' is not recognized as an internal or external command, operable program or batch file.

Haven't followed exactly the PR discussions, I'm wondering what was the rationale for this separation compared to having everything in the .bat file?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nerdvegas/rez/issues/691?email_source=notifications&email_token=AAMOUSQFDVCRHB4W3S3TQBTQEIWMZA5CNFSM4ILF4AF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4EO7WI#issuecomment-520679385, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMOUSQRAQHANFNGN57JMQLQEIWMZANCNFSM4ILF4AFQ .

KelSolaar commented 5 years ago

Ah! We just cross-posted @nerdvegas :]

nerdvegas commented 5 years ago

Hey Thomas, have you had a chance to look further into this?

Cheers A

On Tue, Aug 13, 2019 at 2:04 PM Thomas Mansencal notifications@github.com wrote:

Ah! We just cross-posted @nerdvegas https://github.com/nerdvegas :]

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nerdvegas/rez/issues/691?email_source=notifications&email_token=AAMOUSV43HG3IIOOIPJRD3TQEIXFLA5CNFSM4ILF4AF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4EPH7Y#issuecomment-520680447, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMOUSXVMC2TE2OGPUKGMNDQEIXFLANCNFSM4ILF4AFQ .

KelSolaar commented 5 years ago

Hey @nerdvegas,

I spent a few hours but I, unfortunately, could not get it to work at all. I would probably suggest to revert the PR unless @mottosso has an idea.

Cheers,

Thomas

nerdvegas commented 5 years ago

Ok no worries. I'm off on vacation for a week, but I'll make it a priority to get this reverted when I get back, unless other options present themselves in the meantime.

Thanks for looking into it. A

On Tue, Aug 20, 2019 at 6:16 AM Thomas Mansencal notifications@github.com wrote:

Hey @nerdvegas https://github.com/nerdvegas,

I spent a few hours but I, unfortunately, could not get it to work at all. I would probably suggest to revert the PR unless @mottosso https://github.com/mottosso has an idea.

Cheers,

Thomas

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nerdvegas/rez/issues/691?email_source=notifications&email_token=AAMOUSRAJXKSWXR3XB6YG7TQFL5QNA5CNFSM4ILF4AF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4UFETA#issuecomment-522736204, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMOUSRZ5V4F3R2DV2ECXYLQFL5QNANCNFSM4ILF4AFQ .

instinct-vfx commented 5 years ago

Thanks are a bit busy here currently but i will try to look into it or have someone here look into it in the meantime.

Jordibastide commented 5 years ago

Hi all,

I'm encountering a linked issue with CMakeBuildSystem.build_systems values that results to non quoted generators names in cmake commands

Thanks

bfloch commented 5 years ago

I got it covered in my shell branch. Yes I reverted the commit but also I introduced proper whitespace formatting in cmd.join.

nerdvegas commented 5 years ago

Cheers Blazej, I'll get to this one soon. A

On Tue, Aug 27, 2019 at 1:00 AM Blazej Floch notifications@github.com wrote:

I got it covered in my shell branch. Yes I reverted the commit but also I introduced proper whitespace formatting in cmd.join.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nerdvegas/rez/issues/691?email_source=notifications&email_token=AAMOUSUYY7NKQEXX7ZQ6VULQGPVZ7A5CNFSM4ILF4AF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5EUMOI#issuecomment-524895801, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMOUSUWASC2TMB3NCUNF6TQGPVZ7ANCNFSM4ILF4AFQ .

KelSolaar commented 5 years ago

Looks like this is fixed in https://github.com/nerdvegas/rez/releases/tag/2.47.0

nerdvegas commented 5 years ago

Closed by https://github.com/nerdvegas/rez/pull/698