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

Should now be able to handle spaces in filenames #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Should now be able to handle spaces in filenames #227

wants to merge 1 commit into from

Conversation

SteveSmith16384
Copy link

I've not been able to fully test this, as I don't use Maven NAR directly myself (only as part of a shared project). However, you might find the suggested changes useful.

@@ -386,7 +386,7 @@ public static void makeExecutable(final File file, final Log log) throws MojoExe
if (file.isFile() && file.canRead() && file.canWrite() && !file.isHidden()) {
// chmod +x file
final int result = runCommand("chmod", new String[] {
"+x", file.getPath()
"+x", file.getPath().replaceAll(" ", "\\ ")
Copy link
Member

Choose a reason for hiding this comment

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

Because chmod is called directly rather than using shell sh additional escaping or quoting for spaces shouldn't be required.
Was this required or "added for safety" ?

Copy link
Author

Choose a reason for hiding this comment

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

I seem to remember this being required.

@GregDomjan
Copy link
Member

Thanks @SteveSmith16384

I'd really prefer to avoid managing quotes as it tends to get messier and messier as additional layers are added. But as sh is used it seems it has to be managed.

Did a quick check of the regex used for splitting "[^\\s\"']+|\"[^\"]*\"|'[^']*'"
Found http://stackoverflow.com/questions/366202/regex-for-splitting-a-string-using-space-when-not-surrounded-by-single-or-double

The following comments where interesting, do we need to account for unmatched quotes?

  • The problem with this answer is with unmatched quote: John's mother results splited in [John, s, mother] – leonbloy May 16 '14 at 20:10
  • To fix the issue leonbloy outlines, you can re-order the operands a bit and omit the quotes from the whitespace-group: "([^"]*)"|'([^']*)'|[^\s]+ – Ghostkeeper Sep 15 '14 at 1:26

Thinking we need to make this new version of splitting space separated values into a util function as there seem to be additional locations to potentially fix.
It would have been better I think to make some if not all of these inputs as list still instead of having to deal with issues of string splitting.

Compiler.java
322: final String[] opts = this.optionSet.split("\\s");  
Linker.java
344: final String[] opts = this.optionSet.split("\\s");  
NarGnuConfigureMojo.java
139: gnuBuildconfArgsArray = this.gnuBuildconfArgs.split("\\s");  

Compiler.java (3 matches)
338: final String[] option = optionsProperty.split(" ");  
503: final String[] exclude = defaultExcludes.split(" ");  
548: final String[] include = defaultIncludes.split(" ");  
Linker.java
358: final String[] opt = option.split(" ");  
NarGnuConfigureMojo.java
157: final String[] a = this.gnuConfigureArgs.split(" ");  
NarGnuMakeMojo.java (2 matches)
74: args = this.gnuMakeArgs.split(" "); 
90: args = this.gnuMakeArgs.split(" "); 
NarIntegrationTestMojo.java
952: final List args = Arrays.asList(this.argLine.split(" "));  

@ctrueden ctrueden added this to the unscheduled milestone Jun 14, 2016
@ctrueden
Copy link
Contributor

Kicking to unscheduled, since there seems to be no activity here.

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.

3 participants