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.

Sign up for GitHub

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

Jump to bottom

handling maven path space #754

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

handling maven path space #754

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1181,7 +1181,7 @@
`maven settings.xml found in ${basePath}. Please set the MVN_ARGS environment variable based on the full mvn build command used for this project.\nExample: MVN_ARGS='--settings ${settingsXml}'`
);
}
const mavenCmd = getMavenCommand(basePath, path);
const mavenCmd = '"'+ getMavenCommand(basePath, path)+'"';

Check failure on line 1184 in index.js

View workflow job for this annotation

GitHub Actions / build (16.x) 

Replace `+·getMavenCommand(basePath,·path)+` with `·+·getMavenCommand(basePath,·path)·+·`

Check failure on line 1184 in index.js

View workflow job for this annotation

GitHub Actions / build (21.x, ubuntu-latest) 

Replace `+·getMavenCommand(basePath,·path)+` with `·+·getMavenCommand(basePath,·path)·+·`

Check failure on line 1184 in index.js

View workflow job for this annotation

GitHub Actions / build (18.x) 

Replace `+·getMavenCommand(basePath,·path)+` with `·+·getMavenCommand(basePath,·path)·+·`

Check failure on line 1184 in index.js

View workflow job for this annotation

GitHub Actions / build (20.x) 

Replace `+·getMavenCommand(basePath,·path)+` with `·+·getMavenCommand(basePath,·path)·+·`

Check failure on line 1184 in index.js

View workflow job for this annotation

GitHub Actions / build (21.x) 

Replace `+·getMavenCommand(basePath,·path)+` with `·+·getMavenCommand(basePath,·path)·+·`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @pandiyan-securin. I am not convinced if this is going to be enough to support the path with spaces on Windows fully.

I suspect the real issue could be the use of a single backslash instead of a double backslash.

C:\\Program Files\\Apache Maven\\apache-maven-3.8.4-bin\\mvn

Copy link
Author

@pandiyan-securin pandiyan-securin Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@prabhu , I have checked with double back slashes, still I am getting same kind of issue, Please refer the attached image
cdxgen-path-issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

How is the value specified? Via env args? Can you escape space with a single backslash?

Copy link
Author

Choose a reason for hiding this comment

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

This value is passed via env args and did some hardcoded for testing purpose.
As well path.resolve() returns path with single slash for os portable.
A single backslash () cannot escape a space.

Here are some additional notes from ChatGPT:

  • Double backslashes are usually used in Windows paths, while single backslashes are more common in other operating systems.
  • Using double backslashes can make our code less portable, as it may not work on all platforms.

Copy link
Author

Choose a reason for hiding this comment

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

Update:
Adding maven path to system environment variable is working fine.
When we add a environmental variable MAVEN_HOME for user specific then it throws above error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you so much for looking into this. We can keep the issue open and add support for space (and UNC path) for Windows at some point for all languages and package managers. For now, people can use workarounds such as not using space in the directory names or using WSL, etc.

Copy link
Author

@pandiyan-securin pandiyan-securin Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For your kind information, When maven path has a space then this issue occurs even in WSL.

// Should we attempt to resolve class names
if (options.resolveClass || options.deep) {
const tmpjarNSMapping = collectMvnDependencies(
Expand Down
Loading