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

Remove legacy Javac compilation #694

Conversation

Bencodes
Copy link
Collaborator

@Bencodes Bencodes commented Mar 5, 2022

Notable changes:

  • Removing the legacy compilation so that the new java_common path is the default means for multi-source compilation.
  • Reworked the Jdeps tests to read the test java classes from a java_library output instead of compiling the Java source files by directly calling javac.

@Bencodes Bencodes force-pushed the enable-experimental_use_abi_jars-by-default-and-remove-legacy-javac-compilation branch from bb7833a to 1dab3a1 Compare March 7, 2022 18:48
@Bencodes Bencodes force-pushed the enable-experimental_use_abi_jars-by-default-and-remove-legacy-javac-compilation branch from 14c2982 to ae2b517 Compare March 12, 2022 03:11
@Bencodes Bencodes changed the title Enable experimental_use_abi_jars by default and remove legacy Javac compilation Remove legacy Javac compilation Mar 12, 2022
@@ -90,13 +88,6 @@ class KotlinJvmTaskExecutor @Inject internal constructor(
emptyList()
}
}
context.execute("javac") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks to me that we were compiling java twice, once here and once in https://sourcegraph.com/github.com/bazelbuild/rules_kotlin/-/blob/kotlin/internal/jvm/compile.bzl?L699

I'm still testing this against our code base but so far it seems like it's safe to remove this -- didn't notice huge perf gains but we dont have that much java in our codebase

Copy link
Collaborator

@nkoroste nkoroste left a comment

Choose a reason for hiding this comment

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

This works with our repo. I think it's safe to submit this and close #678 as it will no longer be relevant.

Copy link
Collaborator

@restingbull restingbull left a comment

Choose a reason for hiding this comment

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

We'll need to cut 1.6 alpha candidate to get more testing.

@Bencodes
Copy link
Collaborator Author

@restingbull sounds good. I'm going to merge this one in and we can look into the java_home/rbe issues async d0abebe

@Bencodes Bencodes merged commit 25d4c54 into bazelbuild:master Mar 16, 2022
illicitonion added a commit to illicitonion/rules_kotlin-mixed-compile-repro that referenced this pull request Mar 30, 2022
To repro, run `./run.sh`

Before bazelbuild/rules_kotlin#694 this
succeeded, now it fails.
illicitonion added a commit to illicitonion/rules_kotlin-mixed-compile-repro that referenced this pull request Mar 30, 2022
To repro, run `./run.sh`

Before bazelbuild/rules_kotlin#694 this
succeeded, now it fails.
@Bencodes Bencodes deleted the enable-experimental_use_abi_jars-by-default-and-remove-legacy-javac-compilation branch May 31, 2022 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants