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

baseline-java-versions allows opting in to --enable-preview #2322

Merged
merged 20 commits into from
Jul 29, 2022

Conversation

iamdanfox
Copy link
Contributor

@iamdanfox iamdanfox commented Jul 14, 2022

Before this PR

I had previously made a half-baked attempt to support --enable-preview via gradle-baseline (#1549) but unfortunately it doesn't fully configure Gradle's IntelliJ integration. This isn't really good enough, because it means folks would open a project and see compilation errors.

Also note that literally zero people were using it.

Rather than investing more in this plugin, we've decided to throw it away and merge the functionality into the existing com.palantir.baseline-java-versions plugin.

After this PR

==COMMIT_MSG==
Users of the com.palantir.baseline-java-versions plugin can now set javaVersions { distributionTarget = '17_PREVIEW' } to opt-in to Java's --enable-preview flag at compile time.
==COMMIT_MSG==

After merging this, I'll also need to fix-up the sls-packaging integration. If someone tries to flip this switch before sls-packaging is updated, then their java will fail at startup. This would be caught by dockerized tests (if people have them).

https://github.com/palantir/sls-packaging/blob/1b4c98ce9652d515618ccb426830e14f3df8f34a/gradle-sls-packaging/src/main/java/com/palantir/gradle/dist/service/JavaServiceDistributionPlugin.java#L82-L93

🌶🌶🌶 This functionality is intentionally NOT ALLOWED for projects that publish libraries, because I think the special 'enable preview minor bytecode' is too much of a footgun for consumers to deal with (it requires exactly the same version of java that it was compiled with, so wouldn't allow consumers to upgrade their own java and still use the library)!!!

Possible downsides?

  • The IntelliJ integration works if you use open *.ipr workflow, and it works if you use the IntelliJ-Gradle integration and write features in your main source set, but it does NOT automatically tell IntelliJ to enable-preview on test sourcesets :(. I spent hours trying to make this work, and have not succeeded.
  • The ABI of the extensions is changing, so I'll need to fix up a couple of internal consumers:
            BaselineJavaVersionExtension extension = project.getExtensions().getByType(BaselineJavaVersionExtension.class)
            extension.target().set(JavaLanguageVersion.of(targetVersion))
            extension.runtime().set(JavaLanguageVersion.of(runtimeVersion))

@changelog-app
Copy link

changelog-app bot commented Jul 14, 2022

Generate changelog in changelog/@unreleased

Type
See change types. Select one:

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Users of the com.palantir.baseline-java-versions plugin can now set javaVersions { distributionTarget = '17_PREVIEW' } to opt-in to Java's --enable-preview flag at compile time.

Check the box to generate changelog(s)

  • Generate changelog entry

@iamdanfox iamdanfox force-pushed the dfox/enable-preview2 branch from 83fee7d to 314be58 Compare July 14, 2022 14:35
@iamdanfox iamdanfox marked this pull request as ready for review July 14, 2022 16:10
Node node = provider.asNode()
Node newModuleRootManager = node.component.find { it.'@name' == 'NewModuleRootManager' }
newModuleRootManager.attributes().put("LANGUAGE_LEVEL", new IdeaLanguageLevel(javaVersion).getLevel())
newModuleRootManager.attributes().put("LANGUAGE_LEVEL", chosenJavaVersion.asIdeaLanguageLevel())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a crucial change for Intellij usability - previously it would say something like JDK_17, but now it knows how to say JDK_17_PREVIEW.

I found this by committing all my intellij-related generated files (which are normally gitignored),

then writing some new code:

image

then hitting the button
image

then seeing what changed in my git diff:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

you probably want to delete this module and ban running ./gradlew idea just like gradle/gradle does. There's too many things that go wrong when you're not using gradle integration in intellij these days

Copy link
Contributor Author

@iamdanfox iamdanfox Jul 14, 2022

Choose a reason for hiding this comment

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

@robert3005 I agree the open *.ipr thing seems to have got more fragile (I haven't personally used it in a while)... but with this PR I was specifically trying to make the IntelliJ gradle integration work out of the box!

When I accepted IntelliJ's prompt to enable preview, it added two lines of seemingly non-functional gradle!!

image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the JavaVersion class... I don't understand how the JavaVersion.VERSION_17_PREVIEW thing is supposed to work!

@iamdanfox iamdanfox force-pushed the dfox/enable-preview2 branch from 8b73db0 to 22f43c4 Compare July 14, 2022 17:33
@tpetracca tpetracca requested a review from carterkozak July 29, 2022 13:19
@@ -31,20 +31,22 @@
*/
public class BaselineJavaVersionsExtension {
private final Property<JavaLanguageVersion> libraryTarget;
Copy link
Contributor Author

@iamdanfox iamdanfox Jul 29, 2022

Choose a reason for hiding this comment

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

I'm choosing to literally not support the --enable-preview functionality in this top level javaVersions {} extension, because I think the fact that the generated bytecode CANNOT be run by newer versions of java is such a massive footgun that I don't want folks to ever publish jars with this.

if people really need to break glass, they can still conceivably use the subproject-level javaVersion {} extensions

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

Lgtm. Thanks, @iamdanfox!

bulldozer-bot bot pushed a commit to palantir/witchcraft-api that referenced this pull request Jul 29, 2022
###### _excavator_ is a bot for automating changes across repositories.

Changes produced by the roomba/latest-baseline-oss check.

# Release Notes
## 4.149.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Feature | Users of the `com.palantir.baseline-java-versions` plugin can now set `javaVersions { distributionTarget = '17_PREVIEW' }` to opt-in to Java's `--enable-preview` flag at compile time. | palantir/gradle-baseline#2322 |


## 4.150.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Fix | Fix the `BaselineModuleJvmArgs` plugin to once again work as intended in multi-project builds | palantir/gradle-baseline#2336 |



To enable or disable this check, please contact the maintainers of Excavator.
This was referenced Jul 29, 2022
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.

5 participants