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

gradlew.bat/mvnw.bat should NOT be executable #20513

Merged
merged 1 commit into from
Oct 6, 2021

Conversation

seregamorph
Copy link
Contributor

If the gradle project is generated, the root directory listing has these gradle-wrapper related files:

ls -la demo/gradlew*
-rwxr-xr-x@ 1 morph  staff  8070 Oct  2 21:47 demo/gradlew
-rwxr-xr-x@ 1 morph  staff  2763 Oct  2 21:47 demo/gradlew.bat

The problem is that the second one, the bat file should not be executable, because it is auto-suggested in the shell to be executed, it raises inconvenience and requires manual fixing (via chmod). You can ensure and just list e.g. root directory of quarkus:

ls -la quarkus/mvnw*
-rwxr-xr-x  1 morph  staff  10069 Oct  3 19:50 quarkus/mvnw
-rw-r--r--  1 morph  staff   6607 Oct  3 19:50 quarkus/mvnw.cmd

The first file for *nix has the executable flag, the second file for Windows - does not.

This PR addresses the issue.

The same for maven wrapper bat executable.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 3, 2021

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform labels Oct 3, 2021
@geoand geoand requested a review from ia3andy October 4, 2021 06:32
@ia3andy
Copy link
Contributor

ia3andy commented Oct 4, 2021

Thanks @s-soroosh, can you confirm it won't change anything on Windows?

@gastaldi
Copy link
Contributor

gastaldi commented Oct 4, 2021

Can you squash both commits and let us know that this works on Windows?

@seregamorph
Copy link
Contributor Author

BTW should I fix gradle-wrapper/codestart.yml and maven-wrapper/codestart.yml as well? cc @ia3andy

@ia3andy
Copy link
Contributor

ia3andy commented Oct 4, 2021

BTW should I fix gradle-wrapper/codestart.yml and maven-wrapper/codestart.yml as well? cc @ia3andy

There are two layers depending on if we zip the project or not:
While I am not sure how the executable are dealt for ZIP with on Window (this is why I asked if that wouldn't affect Windows).
I think this is going to be a problem if we remove setExecutable when not zipping on Windows (setExecutable is cross-platform AFAIK, and those files needs to be executable on Window too no?):
https://github.com/quarkusio/quarkus/blob/main/independent-projects/tools/codestarts/src/main/java/io/quarkus/devtools/codestarts/core/strategy/ExecutableFileStrategyHandler.java#L31

@seregamorph
Copy link
Contributor Author

I've played a bit with File.setExecutable(boolean, boolean) on WIndows machine. It seems that it does not have any effect, it's no-op. Even if executable=false is set to files, operating system does not prevent to execute them as usual.
Also I've found related discussion https://coderanch.com/t/567056/java/setExecutable-Windows-Machine that says the same

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 4, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building f2c18cb

Status Name Step Failures Logs Raw logs
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs

@ia3andy
Copy link
Contributor

ia3andy commented Oct 5, 2021

@seregamorph awesome! thanks for the investigation :)

You are good to do both changes!

Copy link
Contributor

@ia3andy ia3andy left a comment

Choose a reason for hiding this comment

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

LGTM

@ia3andy
Copy link
Contributor

ia3andy commented Oct 5, 2021

is the CI failing due to that PR or something else?

@seregamorph
Copy link
Contributor Author

@ia3andy , I cannot give guarantee, but I've checked last main branch commits, most of them have CI/CD objections (red cross):
https://github.com/quarkusio/quarkus/commits/main (JDK 11 Windows as well).

@ia3andy
Copy link
Contributor

ia3andy commented Oct 5, 2021

@seregamorph I've restarted the CI to be sure ;)

@geoand
Copy link
Contributor

geoand commented Oct 6, 2021

@ia3andy feel free to merge is this is good to go

@ia3andy ia3andy merged commit 41a7f69 into quarkusio:main Oct 6, 2021
@quarkus-bot quarkus-bot bot added this to the 2.4 - main milestone Oct 6, 2021
@gsmet gsmet modified the milestones: 2.4.0.CR1, 2.3.1.Final Oct 18, 2021
@gsmet gsmet modified the milestones: 2.3.1.Final, 2.2.4.Final Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/codestarts area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants