PR build kotlin #725

jack-bolles · 2023-08-02T14:11:17Z

Closes #724

What has been done to verify that this works as intended?

both gradle and maven build scripts have been run. Both with explicit failures - to make sure they happen and its not a false positive - and with a working test that passes.

Why is this the best possible solution? Were any other approaches considered?

A stepwise solution. The project should consider committing to one or the other build tool

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

N/A

Do we need any specific form for testing your changes? If so, please attach one.

There are tests in the codebase that rely on kotlin being compiled in the test dir and the source dir.
Manually verified that the source files and the class files are in the appropriate jars.

What hasn't been tested is anything special done by CircleCI in its creating of artefacts. That should be done by a maintainer with the appropriate rights.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

N/A

…otlin 1.8

…he javarosa functionality, and can be removed once the capability to build kotlin from gradle and from maven is completed. Added a kotlin file to `src/test` and to `src/main` to ensure compilation finds appropriate kotlin files and builds them.

…kotlin version 1.8.22

…; doesn't `like The 'java' plugin has been applied, but it is not compatible with the Android plugins.`

jack-bolles · 2023-08-07T11:16:37Z

Couple of questions for the maintainers @seadowg @lognaturel ...

jack-bolles · 2023-08-07T11:17:23Z

First and simplest, are you okay if I bring the gradle version up to 8.2, matching what Collect uses?

jack-bolles · 2023-08-07T11:22:22Z

Second, and more involved....

There's a gradle plugin 'com.android.library' that's tailor-made for this situation. Does the android linting against a jvm library without needing to go multi-platform project (the only other way to get gradle to target both jvm and android code in the same gradle file)

I've got it working locally. It requires ANDROID_HOME to be set on the build machine (or have a local.properties file it can use to find it). Is that acceptable for the CICD process?

jack-bolles · 2023-08-07T11:31:52Z

Separately, the lint options would now be set in build.gradle These are the available options.

I've been testing by playing with these options:

lint {
    htmlOutput = file("lint-report.html")
    textReport = true
    abortOnError = true
    warningsAsErrors = true
    checkDependencies = true
    showAll = true
    enable  'InvalidPackage'
}

I'd want confirmation from the maintainers about which options to specify, and to what values. (e.g. I'm pretty sure that's not where you want the output to go)

seadowg · 2023-08-07T12:14:37Z

First and simplest, are you okay if I bring the gradle version up to 8.2, matching what Collect uses?

Sounds good! No problem with that.

There's a gradle plugin 'com.android.library' that's tailor-made for this situation. Does the android linting against a jvm library without needing to go multi-platform project (the only other way to get gradle to target both jvm and android code in the same gradle file)

I've got it working locally. It requires ANDROID_HOME to be set on the build machine (or have a local.properties file it can use to find it). Is that acceptable for the CICD process?

We already use com.android.library. In build.gradle we either configure the project to use java (and some additional plugins), or com.android.library if the android parameter has been passed. This allows us to run Android tasks (like lint) against the code base and also keep JavaRosa "pure Java" (rather than an Android only library).

Our check-android-api-level CI job uses Circle CI's cimg/android convenience image that has ANDROID_HOME for us so that's not a problem!

I'd want confirmation from the maintainers about which options to specify, and to what values. (e.g. I'm pretty sure that's not where you want the output to go)

I think we're good to just use the default lint options, which I'd hope means we don't have to specify anything.

jack-bolles · 2023-08-07T12:19:52Z

whoops, typo. Sorry.

Not com.android.library. I mean to use com.android.lint instead.

Gradle doesn't like having com.android.library and a kotlin jvm target in the same build (per our conversation on slack). This plugin is the "Gradle Way" of getting around that.

seadowg · 2023-08-07T12:30:45Z

Not com.android.library. I mean to use com.android.lint instead.

Ah I didn't know there was a plugin just for lint! Are we able to just use that and avoid the switching between plugin set hackery then?

…gradle

jack-bolles · 2023-08-07T12:59:05Z

Ah I didn't know there was a plugin just for lint! Are we able to just use that and avoid the switching between plugin set hackery then?

That's what I'm aiming for.

….0; update `TargetSdkVersion` from 30 to 31, as required by Google Play (minSDK still at 21)

jack-bolles · 2023-08-07T14:23:53Z

@seadowg any chance the CI server can upgrade to java 17?

Upgrading gradle -> upgrading android plugin -> J17

A problem occurred evaluating root project 'work'.
> Failed to apply plugin 'com.android.internal.library'.
   > Android Gradle plugin requires Java 17 to run. You are currently using Java 11.
      Your current JDK is located in /usr/lib/jvm/java-11-openjdk-amd64
      You can try some of the following options:
       - changing the IDE settings.
       - changing the JAVA_HOME environment variable.
       - changing `org.gradle.java.home` in `gradle.properties`.

seadowg · 2023-08-07T14:53:06Z

@seadowg any chance the CI server can upgrade to java 17?

Yeah that should be fine. I imagine you can just move to the latest version of cimg/android.

jack-bolles · 2023-08-07T14:58:24Z

@seadowg any chance the CI server can upgrade to java 17?

Yeah that should be fine. I imagine you can just move to the latest version of cimg/android.

I don't have a login for your CI server. One of the maintainers will need to do it, methinks. Unless you want to give me temp access (I prefer not to have it, but I appreciate it might be easier on you)

seadowg · 2023-08-07T15:23:15Z

I don't have a login for your CI server. One of the maintainers will need to do it, methinks. Unless you want to give me temp access (I prefer not to have it, but I appreciate it might be easier on you)

Ah no need to access the CI server. The setup is configured in .circleci/config.yml. You can just make changes there.

…eded by android gradle plugin

jack-bolles · 2023-08-07T19:07:06Z

Have I mentioned how much I hate doing configuration?

That's the upgrading of the gradle and the current supporting plugins. Now that the build image is amenable, I'll move to the com.android.lint plugin.

lognaturel · 2023-08-07T20:50:22Z

🤯 This is amazing, thank you!!

Have I mentioned how much I hate doing configuration?

Really appreciate you taking it on. We dropped trying to get Kotlin in not too long ago because it seemed like a deep rabbit hole. Fantastic to see it coming together.

… for CircleCI to call whilst setting up the replacement android lint task. Necessary because gradle does not support targeting kotlin at both android and the jvm in the same build, so to introduce `com.android.lint`, the `com.android.library` plugin must be removed.

…running a separate build targeted at Android bytecode

…ask in build

…SHOT.jar` and the attributes in the manifest.

jack-bolles · 2023-08-09T17:59:13Z

Hiya - I think I've fulfilled the PR ask, and a bit more.

It's building kotlin code interpolated with java code, both source and test. See src/main/java/org/javarosa/kotlintest/ThisOrThat.kt and src/test/java/org/javarosa/kotlintest/KotlinGradleSetupTest.kt

The gradle build has been updated to use com.android.lint, which doesn't require a separate android build. So the build.gradle file is simpler. And CircleCI doesn't need a separate target. The lint check is now part of the main build. It can be found in the build artefacts, or linked to here ../reports/lint-results.html

Currently, it the lint checks are set to default values, as the previous approach was. After this comment, I'm going to change the setting warningsAsErrors to true, so you can see the build fail automagically. Then revert back to false so that it's mergable.

I've run the maven target mvn package and it seems to do what it did before. I think its a good idea to dry run by one of the maintainers to make sure the full lifecycle behaves as expected.

Changing plugin targets was easier by upgrading gradle to 8.2. This invoved upgrading a couple of the plugins and changing some deprecated syntax in the build.gradle file that had been removed.

…d if they don't pass.

jack-bolles · 2023-08-09T18:07:06Z

@seadowg - ready for review

seadowg · 2023-08-15T12:58:52Z

@jack-bolles sorry I completely missed this that last comment. Will have a look as soon as I can.

seadowg

Amazing!

It looks like there's a few "work in progress" style commits you can get rid of now everything is green:

I wanted to try converting a class to Kotlin (I started CompareChildToAbsoluteExpression) and everything worked like charm in IntelliJ and on a run of ./gradle check. I wasn't able to get mvn package to work however:

[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] <path>/javarosa/src/main/java/org/javarosa/core/model/EqualityExpressionIndexFilterStrategy.java:[55,58] cannot find symbol
  symbol:   class CompareChildToAbsoluteExpression
  location: class org.javarosa.core.model.EqualityExpressionIndexFilterStrategy

Potentially Java classes are not able to access Kotlin ones? Any ideas?

seadowg · 2023-08-15T12:59:33Z

build.gradle

 }

+apply plugin: 'com.android.lint'


Could this not just be in the plugins block above?

It doesn't work if I do. Gradle complains with org.gradle.api.plugins.UnknownPluginException: Plugin [id: 'com.android.lint'] was not found in any of the following sources:

seadowg · 2023-08-15T13:00:38Z

build.gradle

-compileJava.options.encoding = "UTF-8"
-compileTestJava.options.encoding = "UTF-8"
+
+sourceCompatibility = 1.8


I need to confirm this for myself, but I think (and a bunch of the other Java 8 configs in this file) this can be JavaVersion.VERSION_17 now. Android has made this extra confusing as of late.

I defer to your Android knowledge. I erred on the side of caution. LMK what to set it to.

Are you talking about increasing both the sourceCompatibility and the toolchain language version? That would allow for newer features of the language to be used in the code (sourceCompatibility without toolchain would mean use a newer jdk to build but abstain from using newer syntax in the library code)
However, I am guessing the target compatibility will remain as 1.8 right? As long as the binaries are 1.8 compatible then the same set of android devices should be able to use it without issues.

At least that's how I understand it all. I am not familiar with how ODK javarosa works TBH.

Are you talking about increasing both the sourceCompatibility and the toolchain language version?

Yes. The main client for JavaRosa (https://github.com/getodk/collect) already uses Java 17, so I think we're ok to make the bump. I think you're pointing out here that that would mean incompatibility with other projects that are using an older Java toolchain though - that's something we'd need to update in the README.md.

However, I am guessing the target compatibility will remain as 1.8 right? As long as the binaries are 1.8 compatible then the same set of android devices should be able to use it without issues.

I'm pretty sure that targetCompatibility needs to be the same or newer than sourceCompatibility (https://developer.android.com/build/jdks#target-compat).

seadowg · 2023-08-15T16:40:41Z

In response to my earlier comment about the "cannot find symbol" problem: it looks like Maven is less forgiving than Gradle when it comes to a mixed language project according to this SO. I'll spend a bit of time seeing if I can fix it and report back.

seadowg · 2023-08-15T16:48:02Z

It actually looks like @jack-bolles had already setup the pom.xml for a maven mixed language setup. Maybe for maven, we need to use a different source directory for Kotlin (like src/main/kotlin), or potentially being able to reference Kotlin code from Java isn't possible (for maven as we can definitely do this with Gradle)?

jack-bolles · 2023-08-15T18:45:18Z

I was treating interop as a separate PR. To make that work, the ordering of the pom has to change, and the docs would have to be built by dokka instead of javadoc.

Would you like it as part of this PR or as an explicit, separate one?

jack-bolles · 2023-08-15T18:47:50Z

It looks like there's a few "work in progress" style commits you can get rid of now everything is green:

I've always left them in to show the work (both what was and what was not considered) and the evolution.

seadowg · 2023-08-16T10:31:25Z

I was treating interop as a separate PR. To make that work, the ordering of the pom has to change, and the docs would have to be built by dokka instead of javadoc.
Would you like it as part of this PR or as an explicit, separate one?

Right. I'd not anticipated the problems maven would present for interop (although I guess it's not surprising given the prevalence of Gradle in the Kotlin world). I like the idea of taking this as a first step: we get the ability to write code in/convert code to Kotlin providing the Kotlin goes all the way to the "edge" of the library (no Java depends on any Kotlin). That would let us convert everything in the entities package for example (which was what I'd targeted in the issue).

As you suggest in the PR description, it might make more sense to just get rid of Maven rather than go further down the road of getting Kotlin working with it. This would be great for other reasons (no duplication of dependencies, multiple version declarations etc), so I think it's best to get to interop through that route. I'll create an issue for it.

I've always left them in to show the work (both what was and what was not considered) and the evolution.

Makes sense! I'm forgetting we can always squash merge with Github if we want to keep context here, but prevent noise in the master commit history. We definitely want to get rid of 0efd26d however, as it adds extra code we don't want to include in the project.

jack-bolles · 2023-12-01T18:38:58Z

Hi folks, Is there anything stopping this from being accepted (except the build.gradle merge)?

0efd26d is a necessary evil until some of the core code becomes kotlin'd. It's the only code that exercises the fact that the Kotlin-Java interop build is working.

seadowg · 2023-12-04T09:58:34Z

0efd26d is a necessary evil until some of the core code becomes kotlin'd. It's the only code that exercises the fact that the Kotlin-Java interop build is working.

I do see your point about having some Kotlin in there to stop the config regressing. How about converting some existing code rather than introducing new "demo" code? I think EntitiesTest and EntityXFormParserFactory should be safe as they won't be referenced from any of JavaRosa's Java code.

jack-bolles added 4 commits August 2, 2023 13:46

added compileKotlin and compileTestKotlin tasks to gradle targeting K…

fccfe24

…otlin 1.8

Updated Maven's pom.xml to build the kotlin. Upgraded the pom to use …

6d860e2

…kotlin version 1.8.22

pushing to test build.gradle config with circleCI's androidLint setup…

bb341d1

…; doesn't `like The 'java' plugin has been applied, but it is not compatible with the Android plugins.`

jack-bolles added 2 commits August 7, 2023 13:30

upgrade gradle to 8.2; fix deprecated syntax to be current version.

bfbd2d1

re-set build.gradle to before kotlin-for-jvm, isolating upgrade of …

e7e4920

…gradle

upgrade version of com.android.tools.build:gradle from 7.2.1 to 8.1…

d0084e4

….0; update `TargetSdkVersion` from 30 to 31, as required by Google Play (minSDK still at 21)

jack-bolles added 4 commits August 7, 2023 17:24

upgrading CircleCI docker android and jdk images to use Java17, as ne…

a5127df

…eded by android gradle plugin

tweaking CircleCI docker jdk image to use Java17;

16970d8

tweaking CircleCI docker jdk image to use Java17;

e25ed00

tweaking CircleCI docker jdk image to use Java17;

3ac3968

jack-bolles added 5 commits August 9, 2023 12:35

Debug info for test only failing on circleCI

5cc4ad7

Target com.android.lint for Android compatibility checking without …

b8c2b40

…running a separate build targeted at Android bytecode

updated jmh plugin; added jvm toolchain resolver in settings.gradle

9aaccbd

remove lintDebug task from circleCI yaml file. Superseded by lint t…

ffbf6dd

…ask in build

jack-bolles added 2 commits August 9, 2023 17:24

consistent formatting and use of quotation marks

db57299

update the jar task to fix name of artefact to `javarosa-4.3.0-SNAP…

68e50d3

…SHOT.jar` and the attributes in the manifest.

jack-bolles added 2 commits August 9, 2023 19:00

Intentionally failing build to show that Lint checks do fail the buil…

af68af0

…d if they don't pass.

Reverting intentional failure demonstration

7064f6b

seadowg requested changes Aug 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR build kotlin #725

PR build kotlin #725

jack-bolles commented Aug 2, 2023

jack-bolles commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023 •

edited

Loading

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023 •

edited

Loading

jack-bolles commented Aug 7, 2023

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023 •

edited

Loading

lognaturel commented Aug 7, 2023

jack-bolles commented Aug 9, 2023

jack-bolles commented Aug 9, 2023

seadowg commented Aug 15, 2023

seadowg left a comment

seadowg Aug 15, 2023

jack-bolles Aug 15, 2023

seadowg Aug 15, 2023

jack-bolles Aug 15, 2023

corlaez Nov 17, 2023

seadowg Nov 20, 2023

seadowg commented Aug 15, 2023

seadowg commented Aug 15, 2023 •

edited

Loading

jack-bolles commented Aug 15, 2023 •

edited

Loading

jack-bolles commented Aug 15, 2023

seadowg commented Aug 16, 2023

jack-bolles commented Dec 1, 2023

seadowg commented Dec 4, 2023

PR build kotlin #725

Are you sure you want to change the base?

PR build kotlin #725

Conversation

jack-bolles commented Aug 2, 2023

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

jack-bolles commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023 • edited Loading

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023 • edited Loading

jack-bolles commented Aug 7, 2023

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023

seadowg commented Aug 7, 2023

jack-bolles commented Aug 7, 2023 • edited Loading

lognaturel commented Aug 7, 2023

jack-bolles commented Aug 9, 2023

jack-bolles commented Aug 9, 2023

seadowg commented Aug 15, 2023

seadowg left a comment

Choose a reason for hiding this comment

seadowg Aug 15, 2023

Choose a reason for hiding this comment

jack-bolles Aug 15, 2023

Choose a reason for hiding this comment

seadowg Aug 15, 2023

Choose a reason for hiding this comment

jack-bolles Aug 15, 2023

Choose a reason for hiding this comment

corlaez Nov 17, 2023

Choose a reason for hiding this comment

seadowg Nov 20, 2023

Choose a reason for hiding this comment

seadowg commented Aug 15, 2023

seadowg commented Aug 15, 2023 • edited Loading

jack-bolles commented Aug 15, 2023 • edited Loading

jack-bolles commented Aug 15, 2023

seadowg commented Aug 16, 2023

jack-bolles commented Dec 1, 2023

seadowg commented Dec 4, 2023

jack-bolles commented Aug 7, 2023 •

edited

Loading

jack-bolles commented Aug 7, 2023 •

edited

Loading

jack-bolles commented Aug 7, 2023 •

edited

Loading

seadowg commented Aug 15, 2023 •

edited

Loading

jack-bolles commented Aug 15, 2023 •

edited

Loading