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

[#1994] Migrate build.gradle from Groovy to Kotlin #2008

Closed
wants to merge 33 commits into from

Conversation

nseah21
Copy link
Contributor

@nseah21 nseah21 commented Jun 19, 2023

Fixes #1994

Migrate build.gradle from Groovy to Kotlin

The current build script uses Groovy syntax. 

Kotlin has been recommended, however, due to cleaner syntax 
and a better editing experience, owing to its similarity with Java.

Let's refactor the build.gradle file to use Kotlin instead, so that
someone who starts work on build.gradle.kts will find it easier 
to transition to the Java code.

Other information

It is worth noting that builds using Kotlin tend to be slower than builds using Groovy.

However, it is assumed that the added benefit of a more standardised syntax (as mentioned in #1994) outweighs this performance tradeoff.

@nseah21
Copy link
Contributor Author

nseah21 commented Jun 19, 2023

When running gradlew clean build in root directory, the build terminates with an exception after executing the :installFrontend task.

Could I kindly request for some guidance on how to resolve the following issue?

> Task :installFrontend FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':installFrontend'.
> Process 'command 'cmd'' finished with non-zero exit value 1

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

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.6.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 28s
9 actionable tasks: 9 executed

@sikai00
Copy link
Member

sikai00 commented Jun 22, 2023

I ran the changes in your PR with the scan and info flag:

I got this:
image

Starting process 'command '/Users/sikai/RepoSense/build/node/bin/node''. Working directory: /Users/sikai/RepoSense/frontend Command: /Users/sikai/RepoSense/build/node/bin/node /Users/sikai/RepoSense/build/node/lib/node_modules/npm/bin/npm-cli.js ci --production false --cache /Users/sikai/RepoSense/frontend --loglevel info --progress true
Successfully started process 'command '/Users/sikai/RepoSense/build/node/bin/node''
npm info it worked if it ends with ok
npm info using npm@3.3.12
npm info using node@v5.5.0

Usage: npm <command>

versus master branch:
image

This might be a good starting point. Seems like the arguments were not passed into npm directly but rather through /Users/sikai/RepoSense/build/node/lib/node_modules/npm/bin/npm-cli.js ci --production false --cache .

@ckcherry23 ckcherry23 requested review from ckcherry23 and removed request for ckcherry23 June 24, 2023 06:28
@nseah21
Copy link
Contributor Author

nseah21 commented Jul 2, 2023

@sikai00 Thank you for pointing me in the right direction. I have re-attempted the migration and have gotten most of the tasks to run successfully.

However, the task :run seems to have broken, as invoking gradlew run -Dargs="--view --repo <repo-url>" results in the following:

> Task :run
02:21:42 - Config path not provided, using the config folder as default.
02:21:42 - Parsed header of CSV file, repo-config.csv, and recognized columns: ...

instead of:

> Task :run
02:24:29 - Log temp folder has been successfully created
02:24:30 - Cloning in parallel from <repo-url>...

when building with the original build.gradle file.

Has anyone experienced this issue previously when writing the build.gradle in Groovy?

@nseah21 nseah21 marked this pull request as ready for review July 4, 2023 18:26
@ckcherry23 ckcherry23 requested a review from a team July 4, 2023 21:53
@gok99
Copy link
Contributor

gok99 commented Jul 5, 2023

Changes are looking good so far! Don't forget the newline at EOF so that the CI tests can run.

build.gradle.kts Outdated Show resolved Hide resolved
@gok99
Copy link
Contributor

gok99 commented Jul 11, 2023

Bumping the failing build: setCommandLine("cmd") wouldn't work on Linux and Mac. @nseah21 Have you tried using the liferay node plugin we used in the groovy build.gradle?

@yhtMinceraft1010X
Copy link
Contributor

yhtMinceraft1010X commented Jul 16, 2023

@nseah21 Have you tried running ./gradlew clean checkstyleAll test systemTest coverage (must be this exact command, iirc gradlew clean build won't run the coverage task) with --info or --debug flags on your local machine?

The additional logging info might help in determining the exact cause behind Unable to read execution data file /Users/runner/work/RepoSense/RepoSense/build/test-results/systemtest/binary

build.gradle.kts Outdated
tasks.register<JacocoReport>("coverage")

tasks.named<JacocoReport>("coverage").configure {
dependsOn(systemtest, frontendTest)
Copy link
Contributor

Choose a reason for hiding this comment

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

Task dependency should not be introduced here

@github-actions
Copy link
Contributor

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

@sikai00
Copy link
Member

sikai00 commented Aug 22, 2023

Per the error message,

Gradle detected a problem with the following location: 'D:\a\RepoSense\RepoSense\build\jacoco\test.exec'. Reason: Task ':coverage' uses this output of task ':test' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.

it seems the fix to your issue is to declare an explicit dependency for the predefined task 'test'. This follows from coverage using 'test' to know the amount of coverage.

As for the second issue, I did some Googling online. Perhaps this may help?

Hope this helps you fix the issues you are facing!

@MarcusTXK
Copy link
Member

Hi @nseah21, just bumping this PR to check its progress. Could I check if there are any major blockers for you currently?

@nseah21
Copy link
Contributor Author

nseah21 commented Oct 2, 2023

Hi @MarcusTXK, I am still trying to troubleshoot an issue with reading the execution data file.

* What went wrong:
Execution failed for task ':coverage'.
> Unable to read execution data file <file_path>\RepoSense\build\test-results\systemtest\binary

I have attempted the fix posted by @sikai00 above but the build is still throwing the error.

May I request for some guidance on this issue? Thank you.

@damithc
Copy link
Collaborator

damithc commented Oct 29, 2023

@nseah21 I'm not sure if we want to merge this PR in the end, as I mentioned in #1994 (comment)
Even if we don't merge it in the end, it is still fine to continue until we reach a LGTM state so that at least we know what it would look like.

Copy link
Contributor

Hi,
We are going to mark this PR as stale because it has been inactive for the past 30 days.
If no further activity occurs within the following 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 7 days and leave a comment to remove the stale label.
Do let us know if you are stuck so that we can help you!'

@nseah21
Copy link
Contributor Author

nseah21 commented Dec 7, 2023

@gok99 @yhtMinceraft1010X This pull request is ready for the next round of reviews.

@ckcherry23 ckcherry23 requested a review from a team December 8, 2023 02:13
Copy link
Contributor

The following links are for previewing this pull request:

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

Successfully merging this pull request may close these issues.

Gradle: Migrate from Groovy to Kotlin
8 participants