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

Fix windows breaking paths #317

Merged
merged 5 commits into from
Dec 12, 2021

Conversation

swarajsaaj
Copy link
Contributor

Getting the generator run in windows:-

  1. Using "/" as file separator for windows to avoid conflicts with
    mustache.
  2. Avoid Posix file executable for non-posix systems

Note:- If using "\" (File.separator on windows) its not possible to generate the project at all, so replacing it with more universal "/"
Exception in mustache when using "" as file separator from


java.lang.IllegalArgumentException: Illegal character in path at index 9: generator\init\README.md.mustache
	at java.base/java.net.URI.create(URI.java:906) ~[na:na]
	at com.github.mustachejava.resolver.ClasspathResolver.getReader(ClasspathResolver.java:37) ~[compiler-0.9.10.jar:na]
	at com.github.mustachejava.resolver.DefaultResolver.getReader(DefaultResolver.java:48) ~[compiler-0.9.10.jar:na]
	at com.github.mustachejava.DefaultMustacheFactory.getReader(DefaultMustacheFactory.java:118) ~[compiler-0.9.10.jar:na]
	at com.github.mustachejava.MustacheParser.compile(MustacheParser.java:33) ~[compiler-0.9.10.jar:na]

Using "/" as file separator for windows to avoid conflicts with
mustache.
@pascalgrimaud
Copy link
Member

Thanks a lot @swarajsaaj
I didn't find the solution to test with Windows, so your help is welcome here

@jdubois
Copy link
Member

jdubois commented Dec 11, 2021

As some of this code is very close to NubesGen.com you probably have the same errors I had there on Windows. It’s fixed in NubesGen, we should share code on this.

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2021

Codecov Report

Merging #317 (6ede0eb) into main (deb0d8b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##                main      #317   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       456       458    +2     
===========================================
  Files             80        80           
  Lines           1453      1456    +3     
  Branches          32        32           
===========================================
+ Hits            1453      1456    +3     
Impacted Files Coverage Δ
...va/tech/jhipster/lite/common/domain/FileUtils.java 100.00% <100.00%> (ø)
...hipster/lite/generator/project/domain/Project.java 100.00% <100.00%> (ø)
...frastructure/secondary/ProjectLocalRepository.java 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update deb0d8b...6ede0eb. Read the comment docs.

@@ -27,6 +28,8 @@

private final Logger log = LoggerFactory.getLogger(ProjectLocalRepository.class);

private final boolean isPosix = FileSystems.getDefault().supportedFileAttributeViews().contains("posix");
Copy link
Member

Choose a reason for hiding this comment

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

I would move this to a static method in FileUtils, as it's not something which is related to ProjectRepository

Then, if you don't know how to add a unit test, ping me, I can help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @pascalgrimaud ,
Thanks for the review, I have moved the check to FileUtils, and for the test case I have attempted writing a non-windows test case (from a win machine) with mocked static method, let me know if it can be improved? :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes the solution is to use mocked static :-)

@@ -124,7 +128,7 @@ public void setExecutable(Project project, String source, String sourceFilename)

perms.add(PosixFilePermission.OTHERS_READ);
perms.add(PosixFilePermission.OTHERS_EXECUTE);

System.out.println(getPath(project.getFolder()));
Copy link
Member

Choose a reason for hiding this comment

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

just remove this system out and we should be good :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops.
Removed it

@pascalgrimaud pascalgrimaud merged commit ed92928 into jhipster:main Dec 12, 2021
@pascalgrimaud
Copy link
Member

Thanks for your contributions @swarajsaaj
And congrats for keeping the coverage ;)

@swarajsaaj
Copy link
Contributor Author

Thanks for your contributions @swarajsaaj And congrats for keeping the coverage ;)

Thanks for all the help @pascalgrimaud 👍

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

4 participants