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

cmd /C needs to be quoted as a whole when starting integration tests #12910

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@Mpdreamz
Member

Mpdreamz commented Aug 15, 2015

To support spaces in both the command as well as its arguments cmd /C needs
be called like this:

cmd /C ""c:\a b\c.bat" "argument 1" "argument2""

Note the double quotes around the whole command line.

ant was running:

cmd /C "c:\a b\c.bat" "argument 1" "argument2"

which triggers cmd /C to preprocess the command line to

cmd /C c:\a b\c.bat" "argument 1" "argument2

Which would make it appear as though ant was not properly quoting, which it was but just not according to cmd /C's rules.

Closes #12848

cmd /C needs to be quoted as a whole
To support spaces in both the command as well as its arguments cmd needs
be called like this:

cmd /C ""c:\a b\c.bat" "argument 1" "argument2""

ant was running

cmd /C "c:\a b\c.bat" "argument 1" "argument2"

which in windows causes to be preprocessed to

cmd /C c:\a b\c.bat" "argument 1" "argument2

Which would make it appear as though ant was not properly quoting (which
it did sort of).
@dadoonet

This comment has been minimized.

Show comment
Hide comment
@dadoonet

dadoonet Aug 15, 2015

Member

It makes sense and looks good to me.

Member

dadoonet commented Aug 15, 2015

It makes sense and looks good to me.

@s1monw

This comment has been minimized.

Show comment
Hide comment
@s1monw

s1monw Aug 16, 2015

Contributor

LGTM too

Contributor

s1monw commented Aug 16, 2015

LGTM too

@clintongormley

This comment has been minimized.

Show comment
Hide comment
@clintongormley

clintongormley Aug 25, 2015

Member

@Mpdreamz want to merge this and reenable tests with spaces (#12848)?

Member

clintongormley commented Aug 25, 2015

@Mpdreamz want to merge this and reenable tests with spaces (#12848)?

@rjernst

This comment has been minimized.

Show comment
Hide comment
@rjernst

rjernst Aug 25, 2015

Member

IIRC, this get's the ant part working, but there were still integ test failures using spaces. But we should get this in so we can iterate on fixing the spaces problems in the windows bat.

Member

rjernst commented Aug 25, 2015

IIRC, this get's the ant part working, but there were still integ test failures using spaces. But we should get this in so we can iterate on fixing the spaces problems in the windows bat.

@Mpdreamz

This comment has been minimized.

Show comment
Hide comment
@Mpdreamz

Mpdreamz Aug 31, 2015

Member

merged c18093d in master and 2.0

Member

Mpdreamz commented Aug 31, 2015

merged c18093d in master and 2.0

@Mpdreamz Mpdreamz closed this Aug 31, 2015

@Mpdreamz Mpdreamz deleted the Mpdreamz:pr-12710 branch Aug 31, 2015

@clintongormley clintongormley added v2.0.0-beta2 and removed v2.0.0 labels Sep 14, 2015

@clintongormley clintongormley removed the v2.1.0 label Nov 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment