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

* Allow gnuConfigureArgs to accept configure arguments with spaces. #292

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sraza1
Copy link

@sraza1 sraza1 commented Oct 6, 2017

Currently, the arguments are split using space as a delimiter. With this change arguments
containing spaces but specified within quotes are treated as a single argument.

  Currently, the arguments are split using space as a delimiter. With this change arguments
  containing spaces but specified within quotes are treated as a single argument.
@ctrueden
Copy link
Contributor

ctrueden commented Oct 9, 2017

Thanks for submitting this fix, @sraza1!

Would you have time to create or modify one of the integration tests to illustrate this issue? The goal is to have a test which fails prior to your patch, but passes after it.

@sraza1
Copy link
Author

sraza1 commented Oct 9, 2017

Thanks for submitting this fix, @sraza1!
Would you have time to create or modify one of the integration tests to illustrate this issue? The goal is to have a test which fails prior to your patch, but passes after it.

Sure I'll get back with a test case shortly ...

Thanks,
Saqlain

Matcher m = Pattern.compile("([^\"]\\S*|\".+?\")\\s*").matcher(this.gnuConfigureArgs);
// strip double quotes
while (m.find()) {
list.add(m.group(1).replace("\"", ""));
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit imprecise removing any "
Maybe you wouldn't use a " in gnuconfigure arg, or escape one in an arg?

Copy link
Author

@sraza1 sraza1 Nov 6, 2017

Choose a reason for hiding this comment

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

So, keeping in view #291 would searching for the starting " and ending " and stripping them only (if found) be the better way of doing this ?

Copy link
Member

@GregDomjan GregDomjan Mar 20, 2019

Choose a reason for hiding this comment

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

Sorry, totally forgot to come back to this.
Maybe I misread the regex, or it changed?

Could the args include an escaped \" quote? does it even need to be handled, I don't know.

taking the unit test and expanding with some other junk

<gnuConfigureArgs>
		"--with-pkginfo=GNU Executable Integration Test"  arg2 "arg 3"
"arg\" 4"
	</gnuConfigureArgs>

Expected 4 args

  • --with-pkginfo=GNU Executable Integration Test
  • arg2
  • arg 3
  • arg\" 4

Regex provided will come back with 6 matches
1 that's empty spaces/newlines
2-4 that need the leading/trailing " removed
5/6 should actually just be 5 and needing " removed.

I don't do much regex in Java, but I believe from doc this should work, and the capture group should already exclude the "
\s*\"(.*?)(?<!\\)\"\s* or as Java string "\\s*\\"(.*?)(?<!\\\\)\\"\\s*
Checked using https://regex101.com/

@ctrueden
Copy link
Contributor

ctrueden commented Mar 1, 2018

@sraza1 Are you still interested in submitting a test case to validate this bug-fix? It would be very much appreciated.

@ctrueden ctrueden added this to the unscheduled milestone Mar 1, 2018
@sraza1
Copy link
Author

sraza1 commented Mar 1, 2018

@crtueden Yes, I would surely like to do that. Hopefully I'll be able to put up something during the next week...

@sraza1
Copy link
Author

sraza1 commented Mar 7, 2018

@ctrueden I have updated the PR for an integration test. Kindly take a look.

Thanks,
Saqlain

@ctrueden ctrueden removed this from the unscheduled milestone Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants