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

Make JVM sources runnable #17847

Merged

Conversation

chrisjrn
Copy link
Contributor

This adds ./pants run/experimental_run_in_sandbox support for more JVM targets without needing to explicitly declare a deploy_jar. It does that by adapting the existing jvm_artifact rule, which was already fairly generic thanks to the use of ClasspathEntryRequest rules.

The rules will attempt to automatically find a main method through either a JAR manifest (as before for jvm_artifact) or by directly inspecting the contents of the classpath entry JAR returned from compilation. This also permits explicitly declaring a main method in case the other approaches are inconclusive.

@chrisjrn chrisjrn requested review from stuhood and benjyw December 19, 2022 20:21
@chrisjrn chrisjrn marked this pull request as ready for review December 19, 2022 20:21
@chrisjrn
Copy link
Contributor Author

chrisjrn commented Dec 20, 2022

This PR will shrivel up somewhat once #17859 and #17861 land

@chrisjrn chrisjrn force-pushed the chrisjrn/runnable-anything-else-jvm branch from 3d7f431 to 471e138 Compare December 20, 2022 21:51
@chrisjrn
Copy link
Contributor Author

I've rebased this one extensively. Reviewing the commits individually will yield better results than reading the final diff..

@@ -330,9 +372,9 @@ class JunitTestExtraEnvVarsField(TestExtraEnvVarsField):
# -----------------------------------------------------------------------------------------------


class JvmMainClassNameField(StringField):
alias = "main"
class JvmRequiredMainClassNameField(JvmMainClassNameField):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this a subclass rather than a standalone class to reduce churn elsewhere in the codebase.

My instinct is that I could make the new _find_main_by_javap helper into a fully-fledged rule, and we could use that when constructing deploy_jars if main is not specified. When that happens, this subclass can just go away.

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I'm not an expert on the JVM rules, but seems good to me!

@chrisjrn chrisjrn merged commit 672ca1d into pantsbuild:main Dec 22, 2022
@chrisjrn chrisjrn deleted the chrisjrn/runnable-anything-else-jvm branch December 22, 2022 17:39
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.

2 participants