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

Shellcheck shows warnings for the wrapper shell script #26160

Closed
alllex opened this issue Aug 22, 2023 · 7 comments · Fixed by #26171
Closed

Shellcheck shows warnings for the wrapper shell script #26160

alllex opened this issue Aug 22, 2023 · 7 comments · Fixed by #26171

Comments

@alllex
Copy link
Member

alllex commented Aug 22, 2023

Current Behavior

The gradlew wrapper scripts for Gradle 8.3 (and the current 8.4 snapshot version of Gradle) contain warnings detected by Shellcheck integration with IntelliJ IDEA (via bundled Shell Script plugin).

image

Expected Behavior

No shell script warnings are reported

Context (optional)

This is annoying also because IDEA detects those warnings as a part of the commit checks and points the user to them. The users are not supposed to edit the wrapper scripts, so there is no good way of fixing that rather than disabling the Shellcheck, which is most likely not desired.

One check is already ignored for those lines, and I guess we need to add another ignore for the https://github.com/koalaman/shellcheck/wiki/SC2039

Steps to Reproduce

Generate a any project with gradle init and run Shellcheck on the gradlew

Gradle version

8.4 master

Build scan URL (optional)

No response

Your Environment (optional)

No response

@jbartok jbartok added this to the 8.4 RC1 milestone Aug 23, 2023
@jbartok
Copy link
Member

jbartok commented Aug 23, 2023

Thank you for providing a valid report.

The issue is in the backlog of the relevant team, but this area of Gradle is currently not a focus one, so it might take a while before a fix is made.

@alllex
Copy link
Member Author

alllex commented Aug 23, 2023

We have this test that actually runs Shellcheck and should have supposedly failed. I wonder where the disconnect is with what I observe in IntelliJ.

@blindpirate
Copy link
Collaborator

@blindpirate
Copy link
Collaborator

And actually shellcheck doesn't fail on the script on my machine:

# bo @ MacBookProM2 in ~/Projects/gradle/subprojects/plugins/build/tmp/teŝt files/Application.Test/tpsao on git:master x [18:57:38] 
$ cat build/install/sample/bin/sample | grep ulimit
#         * various built-in commands including «command», «set», and «ulimit».
        # In POSIX sh, ulimit -H is undefined. That's why the result is checked to see if it worked.
        MAX_FD=$( ulimit -H -n ) ||
        # In POSIX sh, ulimit -n is undefined. That's why the result is checked to see if it worked.
        ulimit -n "$MAX_FD" ||

# bo @ MacBookProM2 in ~/Projects/gradle/subprojects/plugins/build/tmp/teŝt files/Application.Test/tpsao on git:master x [18:57:40] 
$ shellcheck build/install/sample/bin/sample    

# bo @ MacBookProM2 in ~/Projects/gradle/subprojects/plugins/build/tmp/teŝt files/Application.Test/tpsao on git:master x [18:57:45] 
$ echo $?
0

@blindpirate
Copy link
Collaborator

However, the test should not be skipped on build agents.

@alllex
Copy link
Member Author

alllex commented Aug 24, 2023

And actually shellcheck doesn't fail on the script on my machine:

Yes, I also checked it locally, and to my surprise the test passes. At the same time, the original issue stands and users will see Shellcheck warnings in their projects after upgrading to Gradle 8.3+ wrapper. I believe it's a bad UX.

We could potentially just add the SC2039 to the disabled list, and do a manual validation in the IDE, since the test does not seem to be able to catch it.

@blindpirate
Copy link
Collaborator

blindpirate commented Aug 24, 2023

Try something like this?

...
# shellcheck disable=SC2039
MAX_FD=$( ulimit -H -n )
...

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

Successfully merging a pull request may close this issue.

3 participants