Create the JdwpRemoteDebugPlugin and the Debug errorprones Intellij debug config, to make debugging error-prones easier#181
Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!What happened?Your changelog entries have been stored in the database as part of our migration to ChangelogV3. Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. |
| @@ -0,0 +1,15 @@ | |||
| <component name="ProjectRunConfigurationManager"> | |||
There was a problem hiding this comment.
Is this intentional? I've generally seen run configurations be added by leveraging the idea gradle plugin
There was a problem hiding this comment.
There was a problem hiding this comment.
This is fine for having it in just this repo - however, the real dream for a plugin like this would be you can use it debug errorprone sources too (like gradle-baseline) and get this in the error-prone source repo too automatically. I think you'd need to make another plugin for this though that you apply in the errorprone source (that adds this run configuration). I don't think we currently have a plugin like that atm though.
There was a problem hiding this comment.
Do you mean that we can add some code in RemoteDebugJavaCompilePlugin to create this configuration file in the repo we are running in, e.g. gradle-baseline, if it doesn't exist yet?
| project.getPluginManager().withPlugin("java", unused -> { | ||
| project.getTasks().withType(JavaCompile.class).configureEach(javaCompile -> { | ||
| javaCompile | ||
| .getOptions() | ||
| .getForkOptions() | ||
| .getJvmArgumentProviders() | ||
| .add(new CommandLineArgumentProvider() { | ||
| @Override | ||
| public Iterable<String> asArguments() { | ||
| return List.of("-agentlib:jdwp=transport=dt_socket,server=n,address=localhost:5006"); | ||
| } | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Whenever I've debugged, I've wanted to debug a specific compilation task. If it requires to run another one it depends on, wouldn't this cause me to listen to the wrong one, since it's adding this to all the compilation tasks?
There was a problem hiding this comment.
hmm, i thought you'd only be running one compilation task, no? whats an example where there are multiple compilations being done?
There was a problem hiding this comment.
not necessarily. It'd be the case if you've set up the build-under-debug with multiple projects that compiling java, or a single project with multiple JavaCompile tasks, you can get this if you have different source sets that need compilation.
There was a problem hiding this comment.
We expect the user to apply the debug plugin to the correct subproject, and hence configure the correct javacompile task. is this reasonable?
There was a problem hiding this comment.
Not quite, because I might want to debug the test source set compilation, which typically depends on the main one. It should be enough in most cases though
There was a problem hiding this comment.
if it is going to be used in other repos and not for the tests in suppressible errorprone, then it is confusing if you apply it in a repo that has more than one source set because you will have compileJava, compileTestJava, compileIntegrationTestJava, compile<SourceSet>Java. if you debug anything that's not the main source set, you may get into something weird.
My previous statement when you were devving on this was: it's fine to have a static file, but I assumed incorrectly on how it was supposed to be used. I think this would actually be perfect for having the dynamic pieces:
for each project that this is applied:
- have a unique port for each compile task
- generate a run configuration per project per compile task
so your menu would look like:
debug compile for gradle-suppressible-errorprone:compileJavadebug compile for gradle-suppressible-errorprone:compileTestJava
Not pushing for it in this PR, but it's a thought if you want this to be used widely
There was a problem hiding this comment.
@CRogers has enlightened me that it's in listen mode and its completely fine to have multiple things listen on the same port! ignore me!
| "The JDWP will throw a cryptic error when run with the configuration cache. Turn off configuration" | ||
| + " cache for the build-under-debug. Hint: you can conditionally apply " | ||
| + "`com.palantir.jdwp-remote-debug` only if `IntegrationTestKitBase#isJdwpLoaded()`"); |
There was a problem hiding this comment.
I personally don't know off the top of my head how to disable the configuration cache. It might be good to provide instructions in here as to how to do this? (or even just forcefully disable it?)
There was a problem hiding this comment.
It's only on if you run with --configuration-cache (e.g. using the methods in ConfigurationCacheSpec), and I don't think there's a way to disable it once your build is running :/ would be nice if we could!
There was a problem hiding this comment.
--no-configuration-cache - but yes this should be in error message
| * To debug suppressible-error-prone in a build (works with breakpoints in VisitorStateModifications): | ||
| * 1. Run the "Debug errorprones" debug config which ships with this repository. Notably, it sets up a jdwp listener | ||
| * at port 5006 | ||
| * 2. Run the build with this plugin applied |
There was a problem hiding this comment.
be specific about what build you're referring to i.e. the build the under test
There was a problem hiding this comment.
yep i should make all my terminologies very explicit. thanks for raising this.
| * 2. Run the build with this plugin applied | ||
| */ | ||
| public abstract class JdwpRemoteDebugPlugin implements Plugin<Project> { | ||
| // No other way to programmatically check whether the configuration-cache is on |
There was a problem hiding this comment.
not true!
Use BuildFeatures#getConfigurationCache
There was a problem hiding this comment.
| jdwpRemoteDebugPlugin { | ||
| id = 'com.palantir.jdwp-remote-debug' | ||
| displayName = 'JDWP Remote Debug Plugin' | ||
| description = 'Configures Java compilation tasks to connect to a remote JDWP debugger for debugging ErrorProne and other compiler processes.' | ||
| tags = ['jdwp', 'remote-debug', 'java', 'debugging', 'compiler'] | ||
| implementationClass = 'com.palantir.gradle.suppressibleerrorprone.JdwpRemoteDebugPlugin' | ||
| } |
There was a problem hiding this comment.
is this something you want to publish? or you only want it for your tests. If the latter, we should not publish this here.
There was a problem hiding this comment.
If the former, then it's probably a bit weird to publish within gradle-suppressible-error-prone
There was a problem hiding this comment.
We want to publish this. @aldexis wants to use it in e.g baseline as well
There was a problem hiding this comment.
if that's the case, then we should move it to gradle-utils, currently, you need to depend on gradle-suppressible-errorprone to get this, which is weird.
There was a problem hiding this comment.
To be honest, I would even prefer for this to just be a configuration option in suppressible-error-prone that I can just turn on
There was a problem hiding this comment.
hmm we only use this debugging plugin when we also have suppressible-error-prone, is it general enough to be put in gradle-utils?
There was a problem hiding this comment.
I think the point is you can just slap this plugin on a repo using suppressible-error-prone without having to add another dependency to your buildscript - not really to be used as a generic compiler debug plugin used by other plugins.
There was a problem hiding this comment.
okay, that makes a bit more sense. Let's add this to the README as well pls 🙏
| } | ||
| }) | ||
| } | ||
| apply plugin: 'com.palantir.jdwp-remote-debug' |
There was a problem hiding this comment.
I like that you've encapsulated this in a plugin, pretty neat
| // To debug the tests in this file | ||
| // 1. Run the "Debug errorprones" debug configuration, which ships with this repository | ||
| // 2. Go to the test you're looking for and run debug on it as well, but without any special configurations | ||
| debug = isJwdpLoaded() |
There was a problem hiding this comment.
There was a problem hiding this comment.
yeah, but it sets it only when you're trying to create a runner, so your test code can't depend on it as it runs too late. It's kinda shit that they set it inside createRunner
There was a problem hiding this comment.
Ah, you mean if you want to do something different depending on if debug is set.
There was a problem hiding this comment.
It does, but only at runner creation time, which is too late unfortunately. we need it at setup time, when we create the build script
There was a problem hiding this comment.
so we have to set it here early. i should explain this in a comment.
|
This PR has been automatically marked as stale because it has not been touched in the last 14 days. If you'd like to keep it open, please leave a comment or add the 'long-lived' label, otherwise it'll be closed in 7 days. |
Before this PR
In this repo, and every repo which consumes suppressible-error-prone, to debug errorprones, we have to
After this PR
Simplify step 1 by creating a debug config file, which can also be replicated to other repositories like gradle-baseline
Also, simplify step 2 by exporting a gradle plugin which does the above for you.
==COMMIT_MSG==
Create the
JdwpRemoteDebugPluginand theDebug errorpronesIntellij debug config, to make debugging error-prones easier==COMMIT_MSG==
Possible downsides?
Removing step 1 would be a game changer. I tried to do this with a compound task which ran both the jdwp listener and the test build, but there was no way to run a compound task which runs a specific test case, like how intellij allows you to run test cases from the gutter.