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

NETBEANS-3254 Remove allowAmbiguousCommands flag #1849

Merged
merged 2 commits into from Jan 18, 2020

Conversation

lhochet
Copy link
Contributor

@lhochet lhochet commented Jan 5, 2020

Removes the jdk.lang.Process.allowAmbiguousCommands flag from netbeans.conf.

Instead defines a NETBEANS_MAVEN_COMMAND_LINE environment variable
local to the ProcessBuilder used to launch Maven, and pass a reference to that
variable on the cmd.exe command line.

With that change, the quote prefixing and suffixing of the Maven command
line prevented the command line from being properly launched so it was removed.
(Tested both with the embedded Maven 3.3.9 and an external Maven 3.6.3)

Removes the jdk.lang.Process.allowAmbiguousCommands flag netbeans.conf.

Instead defines a NETBEANS_MAVEN_COMMAND_LINE environment variable
local to the ProcessBuilder used to launch Maven, and pass a reference to that
variable on the cmd.exe command line.

With that change, the quote prefixing and suffixing of the Maven command
line prevented the command line from being properly launched so it was removed.
(Tested both with the embedded Maven 3.3.9 and an external Maven 3.6.3)
@ebarboni
Copy link
Contributor

Hi @lhochet, how to test that ?

@ebarboni ebarboni added this to the 11.3 milestone Jan 16, 2020
@lhochet
Copy link
Contributor Author

lhochet commented Jan 17, 2020

Hello @ebarboni,
I think these steps would demonstrate the fix:

  1. create a test branch off master
  2. clean build NetBeans
  3. remove -J-Djdk.lang.Process.allowAmbiguousCommands=true from etc/netbeans.conf
  4. run NetBeans with a JDK post that was release in October or earlier this week (8 or above 11 -- my own test, 15+6)
  5. create a new Maven Java Application, add a Main that prints hello world
  6. try to build it
  7. expected error: Cannot run program "cmd" (in directory "D:\dev\tests\mavenproject1"): Malformed argument has embedded quote: "C:\softs\apache-maven-3.6.3\bin\mvn.cmd" -Dmaven.ext.class.path=E:\netbeans\apache-netbeans\nbbuild\netbeans\java\maven-nblib\netbeans-eventspy.jar -Dfile.encoding=UTF-8 install
  8. merge the branch for this PR (I think adding my fork of NetBeans as a remote will expose the branch)
  9. rebuild NetBeans
  10. run NetBeans again
  11. retry to build the Maven project
  12. double check by then trying to run the project

@ebarboni
Copy link
Contributor

works, allowAmbiguousCommands is only for this maven execution ?

@neilcsmith-net
Copy link
Member

@ebarboni it was certainly only added late in 11.2 because of reports of this bug with Maven on the JDK updates that had just been released. I don't know whether other things might be affected though - Maven was very obvious - but IDE wasn't tested without that option for very long!

Nice fix btw!

@ebarboni
Copy link
Contributor

@neilcsmith-net thanks for comment.
@lhochet can you consider reverting the nb/ide.launcher/netbeans.conf changes.( to avoid potential regression for others)
Fix for maven itself is ok to merge

…e as it could be of use for non Maven contexts
@lhochet
Copy link
Contributor Author

lhochet commented Jan 18, 2020

@ebarboni sure, done.

It could still be of use to external processes (.bat/.cmd particularly) being launched from NetBeans and having double quote(s) within double quoted arguments.
eg.
cmd /c ""path to maven with spaces/mvn.cmd" install"
or
cmd /c C:\softs\apache-maven-3.6.3\bin\mvn.cmd install -Dfoo="bar team"
(though this later one will fail because it will be passed by Java as cmd.exe" /c "C:\softs\apache-maven-3.6.3\bin\mvn.cmd -Dfoo="bar team" install)

@ebarboni ebarboni merged commit f1dd1d8 into apache:master Jan 18, 2020
@ebarboni
Copy link
Contributor

maybe we should remove just after 11.3 release to see on longer period

@neilcsmith-net
Copy link
Member

@ebarboni that would be good and NetCAT for 12.0 should hopefully catch any other issues.

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.

None yet

3 participants