PR build kotlin

[jack-bolles]

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

[jack-bolles]

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

[jack-bolles]

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

[jack-bolles]

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]

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]

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]

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]

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?

[jack-bolles]

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.

[jack-bolles]

@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]

@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]

@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]

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.

[jack-bolles]

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]

🤯 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.

[jack-bolles]

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.

[jack-bolles]

@seadowg - ready for review

[seadowg]

@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:

• bb341d1
• 0efd26d
• 5cc4ad7
• e7e4920
• eb71ddd
• af68af0
• 7064f6b

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]

Could this not just be in the plugins block above?

[jack-bolles]

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]

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.

[jack-bolles]

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

[corlaez]

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.

[seadowg]

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]

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]

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]

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]

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]

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]

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]

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.