Skip to content

Back to Shell usage for Windows command line (+ minor Win UT fix)#20

Closed
axel3rd wants to merge 1 commit intocodehaus-plexus:masterfrom
axel3rd:#17
Closed

Back to Shell usage for Windows command line (+ minor Win UT fix)#20
axel3rd wants to merge 1 commit intocodehaus-plexus:masterfrom
axel3rd:#17

Conversation

@axel3rd
Copy link
Contributor

@axel3rd axel3rd commented Feb 7, 2017

PR for #17 : Back to Shell usage for Windows command line (only).
Allow embedded CMD command (echo, ...) and binary on PATH usage.
Regression since v3.0.15 (b38a1b3 : improvement avoiding some shell injection).

In addition, minor UT fix on DirectoryScanner (were platform dependant)

michael-o
michael-o reviewed Jun 18, 2017
View reviewed changes
src/main/java/org/codehaus/plexus/util/cli/Commandline.java
* Shell is autodetected from operating system
*
* Shell usage is only desirable when generating code for remote execution.
* For Linux, shell usage is only desirable when generating code for remote execution.
Copy link
Member

@michael-o michael-o Feb 7, 2017

Choose a reason for hiding this comment

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

Not Linux, but Unix-like. There are other OSes too.

Copy link
Contributor Author

@axel3rd axel3rd Jun 18, 2017

Choose a reason for hiding this comment

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

@michael-o : Good catch, you are right. Do you want I update the PR (there is 6 mentioned in file) ? (I'm not sure to understand, you have closed the PR).

Copy link
Member

@michael-o michael-o Jun 18, 2017

Choose a reason for hiding this comment

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

I have already merged a fixed version of the PR.

Copy link
Contributor Author

@axel3rd axel3rd Jun 18, 2017

Choose a reason for hiding this comment

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

ok, thanks

@michael-o michael-o closed this Jun 18, 2017
@axel3rd axel3rd deleted the #17 branch June 18, 2017 21:19
@rfscholte
Copy link
Member

rfscholte commented Jun 19, 2017

When trying to run this with gitbash (mingw64) I get the following result:

Failed tests:
  DirectoryScannerTest.testFollowSymlinks:186 null
  DirectoryScannerTest.testFollowSymlinksFalse:155 expected:<9> but was:<11>

Tests run: 303, Failures: 2, Errors: 0, Skipped: 0

@axel3rd axel3rd mentioned this pull request Jun 19, 2017
Closed
@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 19, 2017

@rfscholte : Thanks for the feedback.

I have tested on 3.0.24 (GitBash, Windows 10), this two tests fail previously your report (on the assertAlwaysIncluded):

DirectoryScannerTest.testFollowSymlinks:185->assertAlwaysIncluded:167 nul
DirectoryScannerTest.testFollowSymlinksFalse:154->assertAlwaysIncluded:167 null

=> I have open #24, I will try to have a look on it.

@axel3rd axel3rd restored the #17 branch June 20, 2017 09:15
@axel3rd axel3rd deleted the #17 branch June 20, 2017 10:01
@axel3rd
Copy link
Contributor Author

axel3rd commented Jun 20, 2017 •
edited
Loading

@rfscholte : After investigation, the unit tests fail when symlink resources are not preserved during git clone.

The core.symlinks option should be used:

git clone -c core.symlinks=true https://github.com/codehaus-plexus/plexus-utils.git

I will reorient #24 and provide a PR for this check, to avoid some people loose time like us next time.

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

Reviewers

@michael-o michael-o michael-o left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@axel3rd @rfscholte @michael-o