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

OS independent call to applyStyle #313

Merged
merged 8 commits into from Dec 12, 2022
Merged

Conversation

reger24
Copy link
Contributor

@reger24 reger24 commented Dec 2, 2022

Problem: While working on a Windows system the supplied linux script applyStyle.sh won't work

Solution: OS independent call to applyStyle using gradle features, JavaExec could be used to call the google-java-format.jar directly (supplied change)

Alternative: could be a Wndows batch script simulating the linux script and a gradle if (Os.isFamily(Os.FAMILY_WINDOWS) .... to call the *.bat instead of .sh
The content of the applyStyle.bat file could be
for /R ../app/src/main/java/org/openbot %%f in (
.java) DO (java -jar google-java-format-1.7-all-deps.jar -r %%f)

My preference is JavaExec.

Problem: While working on a Windows system the supplied linux script applyStyle.sh won't work

Solution: OS independent call to applyStyle using gradle features, JavaExec could be used to call the google-java-format.jar directly (supplied change)

Alternative:  could be a Wndows batch script simulating the linux script and a gradle
if (Os.isFamily(Os.FAMILY_WINDOWS) .... to call the *.bat instead of *.sh

My preference is JavaExec.
@thias15
Copy link
Collaborator

thias15 commented Dec 2, 2022

Can't you run the applyStyle task using the provided gradlew.bat script? So with the JavaExec route, how would you call the applyStyle task in each OS (Windows, Linux, Mac)? Did you notice any performance differences?

@thias15 thias15 added this to In progress in OpenBot via automation Dec 2, 2022
@reger24
Copy link
Contributor Author

reger24 commented Dec 2, 2022

Can't you run the applyStyle task using the provided gradlew.bat script? So with the JavaExec route, how would you call the applyStyle task in each OS (Windows, Linux, Mac)? Did you notice any performance differences?

@thias15 , no gradlew.bat won't work as it currently try's to call utils/applyStyle.sh (which will not execute on Windows).
to call applyStyle (via JavaExec) would be just as described in docu with the gradlew script for all OS (or via IDE gradle applyStyle)
Example:
cd android
gradlew applyStyle

Output:
Welcome to Gradle 7.1.1!

Here are the highlights of this release:

  • Faster incremental Java compilation
  • Easier source set configuration in the Kotlin DSL

For more details see https://docs.gradle.org/7.1.1/release-notes.html

Starting a Gradle Daemon, 1 incompatible Daemon could not be reused, use --status for details

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.1.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 8s
1 actionable task: 1 executed

Regarding the performance, I can't compare (from the example you see it took 8s with JavaExec)

@thias15
Copy link
Collaborator

thias15 commented Dec 3, 2022

Tested on Mac, there it fails. Will test on Linux and Windows when I get a chance.

java.lang.IllegalArgumentException: Unsupported class file major version 61

This points to a problem with the Java version, but on master it works fine.

@reger24
Copy link
Contributor Author

reger24 commented Dec 3, 2022

Good, thx,

P.S. a general possible problem with both (JavaExec and the shell script) could be (possibly in future), that applyStyle always goes through all *.java source files which ends up in a pretty long argument list for the call to the jar (but this general issue has nothing to do with the failure on Mac, where I agree is likely some Java version issue).

function as before,
just removed the not deeded convert of Filetree to list and let gradle use directly use the source file name (without not needed toString() call.
@thias15
Copy link
Collaborator

thias15 commented Dec 10, 2022

Sorry for the delay. With the correct Java version setup it works fine. Thanks! How about doing something similar for checkStyle to have cross-platform support? Any ideas, how to do better than going through all .java files?

@reger24
Copy link
Contributor Author

reger24 commented Dec 11, 2022

Tried it already also for 'checkStyle' but didn't find a way to cope current output, problem was/is that the script uses the java exitcode to output different text and I haven't found a way (there is currently no way) to catch the exitcode within the gradle task to do something similar.
So what I can offer is a somewhat close solution (but due to above didn't ad to this pullrequest)

  1. with slightly different text output (see example) on files found with wrong style
  2. and never tested if your workflow script will handle the FAIL of 'checkstyle' correct

Here the code:

-----start code in build.gradle--------------------
task checkStyle(type:JavaExec) {
workingDir = "utils"
classpath = files("utils/google-java-format-1.7-all-deps.jar")
FileTree tree = fileTree('app/src/main/java') {
include '**/*.java'
}

args(["-n","--set-exit-if-changed"]) // -n prints filename if using wrong style

tree.each {args += it}

doFirst {
    println("The following java files follow the wrong style. Please use ./gradlew applyStyle")
}
doLast { // will only execute on success (exitcode=0)
    println ("-> All java files follow the correct style.")
}

}
-----eof code-----------------------------

Example 1 with simulated wrong format in one File
------------ start Example 1 ---------------------

Task :checkStyle FAILED
The following java files follow the wrong style. Please use ./gradlew applyStyle
android\app\src\main\java\org\openbot\modelManagement\BackHandlingFilePickerFragment.java

Execution failed for task ':checkStyle'.

Process 'command '\Program Files\Android\Android Studio\jre\bin\java.exe'' finished with non-zero exit value 1
------------ eof Example 1 -------------------

Example 2 output with all files with correct style
------- start Example 2 ---------------

Task :checkStyle
The following java files follow the wrong style. Please use ./gradlew applyStyle
-> All java files follow the correct style.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.4/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1s
1 actionable task: 1 executed
---------- eof Example 2 -----------------

I've no better idea as going to all .java files, as gradle doesn't currently offer a way to work only on modified files (have read it's on their todo list). As long as the command-line length limit is not exeeded the current way is performance wise good.
The alternative to call checkstyle and/or applyStyle on each .java file separat would prevent the in future possible issue but has dramatic bad effect on the performance.

@thias15 thias15 self-assigned this Dec 11, 2022
@thias15
Copy link
Collaborator

thias15 commented Dec 11, 2022

Above looks good. Locally, even having just applyStyle would be sufficient, but it's kind of nice to also have the checkStyle option. I can try if the workflow still works. Otherwise, we could also keep the old Exec tasks and sh scripts, just rename to task checkStyleUnix(type:Exec) and task applyStyleUnix(type:Exec).

@thias15
Copy link
Collaborator

thias15 commented Dec 11, 2022

Workflow works fine. Can you double-check that both checkStyle and applyStyle work on Windows? Then we can merge.

@reger24
Copy link
Contributor Author

reger24 commented Dec 11, 2022

On windows both work fine.
I'll update the pull request with

  1. the checkStyle code
  2. your suggestion to keep old task with Unix appended,

will do it right now.
cu

reger24 and others added 5 commits December 12, 2022 00:25
…applystyle

# Conflicts:
#	android/build.gradle
reapplied the collorcode

apply JavaExec to applyStyle,
as fallback keep both renamed shell script based tasks as checkStyleUnix and applyStyleUnix
OpenBot automation moved this from In progress to In review Dec 12, 2022
@thias15 thias15 merged commit bbaacd0 into isl-org:master Dec 12, 2022
OpenBot automation moved this from In review to Done Dec 12, 2022
@reger24 reger24 deleted the applystyle branch December 13, 2022 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
OpenBot
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants