Fix splitting of the build_flags. #4580

Merged
merged 1 commit into from May 14, 2017

Conversation

Projects
None yet
3 participants
@ensonic
Contributor

ensonic commented May 12, 2017

With this change one can supply more complex build flags as demonstrated by the test.

Problem

Previously the option handling did not properly un-escape the flags.
Fixes #4563

Solution

Use safe_shlex_split() instead of spit() and handle top-level quotes.

Fix splitting of the build_flags.
With this change one can supply more complex build flags as demonstrated by the test.

Fixes #4563
@stuhood

Thanks Stefan.

I still think that converting this to a list option might be less error prone, and provide other benefits (like the ability to extend the set of options across multiple config files: see http://www.pantsbuild.org/options.html#list-options).

What are your thoughts on the matter?

@@ -58,7 +60,8 @@ def execute(self):
vt.target.import_path + '.a')
def _go_install(self, target, gopath):
- args = self.get_options().build_flags.split() + [target.import_path]
+ build_flags = re.sub(r'^"|"$', '', self.get_options().build_flags)

This comment has been minimized.

@stuhood

stuhood May 12, 2017

Member

Can you add a comment indicating what this is replacing?

@stuhood

stuhood May 12, 2017

Member

Can you add a comment indicating what this is replacing?

This comment has been minimized.

@baroquebobcat

baroquebobcat May 12, 2017

Contributor

If we're going to go down the shlex split path, I think it might be worth it to extract a helper for doing the post-processing on the arguments and write unit tests for it.

For example, I think that the starting and ending quotes may be taken care of by the option parser, so this may not be necessary.

@baroquebobcat

baroquebobcat May 12, 2017

Contributor

If we're going to go down the shlex split path, I think it might be worth it to extract a helper for doing the post-processing on the arguments and write unit tests for it.

For example, I think that the starting and ending quotes may be taken care of by the option parser, so this may not be necessary.

@ensonic

This comment has been minimized.

Show comment
Hide comment
@ensonic

ensonic May 12, 2017

Contributor

TBH I don't like the option part at all. It makes it hard to see what option you are actually passing. In the context of an config file it might be okay (like in a yaml file, sure make it a list, since it is a list). But here it is a list of options that belong together and are build_flags.

I agree that the quote removal should probably go to the options parser. But then I don't see why we'd need a helper method beyond what safe_shlex_split() does.

Contributor

ensonic commented May 12, 2017

TBH I don't like the option part at all. It makes it hard to see what option you are actually passing. In the context of an config file it might be okay (like in a yaml file, sure make it a list, since it is a list). But here it is a list of options that belong together and are build_flags.

I agree that the quote removal should probably go to the options parser. But then I don't see why we'd need a helper method beyond what safe_shlex_split() does.

@stuhood

This comment has been minimized.

Show comment
Hide comment
@stuhood

stuhood May 12, 2017

Member

In the context of an config file it might be okay (like in a yaml file, sure make it a list, since it is a list). But here it is a list of options that belong together and are build_flags.

I think the expectation is that options like these are generally passed via pants.ini rather than on the command line... in that context, the list option would look something like:

[compile-go]
build_flags: [
    '--ldflags \'-extldflags "-static"\'"'
  ]

(...I might not have manually lexed that correctly, but you get the idea.)

Member

stuhood commented May 12, 2017

In the context of an config file it might be okay (like in a yaml file, sure make it a list, since it is a list). But here it is a list of options that belong together and are build_flags.

I think the expectation is that options like these are generally passed via pants.ini rather than on the command line... in that context, the list option would look something like:

[compile-go]
build_flags: [
    '--ldflags \'-extldflags "-static"\'"'
  ]

(...I might not have manually lexed that correctly, but you get the idea.)

@ensonic

This comment has been minimized.

Show comment
Hide comment
@ensonic

ensonic May 13, 2017

Contributor

@stuhood feel free to do it. I am glad if this gets fixed and into a release.

Contributor

ensonic commented May 13, 2017

@stuhood feel free to do it. I am glad if this gets fixed and into a release.

@stuhood stuhood merged commit 268c326 into pantsbuild:master May 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood added this to the 1.3.0 milestone May 14, 2017

@ensonic ensonic deleted the ensonic:bug_4563 branch May 15, 2017

stuhood added a commit that referenced this pull request May 15, 2017

Fix splitting of the build_flags. (#4580)
### Problem

Previously the option handling did not properly un-escape the flags.
Fixes #4563

### Solution

Use safe_shlex_split() instead of spit() and handle top-level quotes.

### Result

With this change one can supply more complex build flags as demonstrated by the test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment