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

[JENKINS-40109] Pre-evaluate compilation errors to make them serializable #53

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@rudolfwalter
Copy link
Contributor

rudolfwalter commented Sep 8, 2018

One way to solve JENKINS-40109.

I have one small uncertainty: do you think anyone could be relying on the details inside the MultipleCompilationErrorsException? Should we save it as a transient field inside CompilationErrorException (and override getCause() to return it)?

@rudolfwalter

This comment has been minimized.

Copy link
Contributor Author

rudolfwalter commented Sep 13, 2018

Pinging @abayer (assignee of the ticket)

@dwnusbaum
Copy link
Member

dwnusbaum left a comment

Thanks for looking at the issue! This seems like a good approach to improving the error messages, although it also seems like some of the problems are with plugins that shouldn't be serializing the exceptions in the first place.

Did you try reproducing the issue in a test? If not, I think that an automated test would be very helpful for this issue.

Show resolved Hide resolved ...org/jenkinsci/plugins/workflow/cps/global/CompilationErrorException.java
*/
public class CompilationErrorException extends RuntimeException {
public CompilationErrorException(MultipleCompilationErrorsException original) {
super(original.toString());

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Sep 13, 2018

Member

nit: I'd have to check what the exact output is to know for sure, but my first guess is that it would be better to use original.getMessage() instead of original.toString().

This comment has been minimized.

Copy link
@rudolfwalter

rudolfwalter Sep 13, 2018

Author Contributor

That is what I meant to call, except I mistyped last second. Nice catch.

Show resolved Hide resolved ...org/jenkinsci/plugins/workflow/cps/global/CompilationErrorException.java

@dwnusbaum dwnusbaum requested review from dwnusbaum, joseblas and abayer Sep 13, 2018

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Sep 13, 2018

do you think anyone could be relying on the details inside the MultipleCompilationErrorsException

I'm not sure. Maybe our exception could extend CompilationFailedException or its superclass instead so we could attach more of the information from the original exception, but if none of that shows up in the message in the first place then I doubt anyone is relying on it.

@rudolfwalter

This comment has been minimized.

Copy link
Contributor Author

rudolfwalter commented Sep 13, 2018

I'll write a test to showcase the problem.

Also, we cannot extend CompilationFailedException because it has non-serializable members.

@dwnusbaum
Copy link
Member

dwnusbaum left a comment

This looks like a good fix to me, although I'm not sure if it would address the issue @abayer noted in the ticket about compilation failures for files in the src directory. Even so, this looks like a decent improvement to me, and we should be able to handle that case separately.

It looks like we could instead make the fix in CpsScript in workflow-cps, but I think this should be specific to UserDefinedGlobalVariables, so it makes sense to me to fix it here, but I would appreciate if someone who understands the big picture better than I do could confirm.

sampleRepo.git("commit", "--message=init");
GlobalLibraries.get().setLibraries(Collections.singletonList(new LibraryConfiguration("badlib", new SCMSourceRetriever(new GitSCMSource(sampleRepo.toString())))));
WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
p.setDefinition(new CpsFlowDefinition("@Library('badlib@master') _; try { mymagic(); echo 'why are we here?' } catch(err) { echo 'forcing CPS serialization...'; sleep 1; echo 'did we die?' }", true));

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Sep 19, 2018

Member

nit: Might be helpful to use echo err.getMessage() in the first catch block and then r.assertLogContains("syntax error") to make it obvious what error is expected to be thrown there.

This comment has been minimized.

Copy link
@rudolfwalter

rudolfwalter Sep 25, 2018

Author Contributor

Done.

@jglick jglick self-requested a review Sep 25, 2018

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Oct 1, 2018

Tested this out locally today to verify that without the fix the new test fails with a NotSerializableException, which it did. The CI build appears to be failing because of an infra-related issue, and the tests pass for me locally.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Oct 1, 2018

fyi, I relaunched the CI build.

@abayer
Copy link
Member

abayer left a comment

How will this work for a compilation failure in src?

* lead to the user being presented with a {@link NotSerializableException} instead
* of a compilation error -- see JENKINS-40109).
*/
public class CompilationErrorException extends RuntimeException {

This comment has been minimized.

Copy link
@abayer

abayer Oct 1, 2018

Member

Any chance we could rename this to, say, CpsCompilationErrorException and move it to workflow-cps-plugin? I'd like it to be clear in the name that this is specifically for CPS-compilation errors, and we're going to want to have the same kind of logic for the load step over in workflow-cps-plugin too.

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Oct 1, 2018

Member

Yeah that approach makes sense to me. I'm happy to do this bit if @rudolfwalter is fine with it.

import java.util.Collections;

@Issue("JENKINS-40109")
public class CompilationErrorExceptionTest extends Assert {

This comment has been minimized.

Copy link
@abayer

abayer Oct 1, 2018

Member

Why are you extending Assert here?

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Oct 1, 2018

Member

Probably just a copy-paste from elsewhere in the plugin (

), but yeah it would be good to clean it up.

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Oct 1, 2018

How will this work for a compilation failure in src?

I did some quick testing, and it looks like there are 2 cases:

  • When using @Library and importing the classes with syntax errors, the compilation errors in src are thrown straight out of WorkflowRun#run, so we don't need to worry about the exception being serialized as part of the program in that case. (Although we could still convert the exception for consistency.)
  • When using the library step, the compilation errors in src are thrown out of LibraryStep$LoadedClasses#loadClass, so we'll need to add a catch clause for MultipleCompilationErrorsException there as well.

I'll open a PR in workflow-cps to move the exception and a new one in this repo on top of this PR to handle the src cases as well.

@rudolfwalter

This comment has been minimized.

Copy link
Contributor Author

rudolfwalter commented Oct 1, 2018

@dwnusbaum Should I make the changes @abayer requested, or will your PR replace this one completely and you'll do them there?

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Oct 1, 2018

@rudolfwalter My plan was to file a new PR in both places, but the new PR here would still have all of your commits (so you'll still have attribution on them), but if you'd rather do it yourself, that's totally fine too!

@rudolfwalter

This comment has been minimized.

Copy link
Contributor Author

rudolfwalter commented Oct 1, 2018

I just wanted to clarify if there was anything else I needed to do. Go right ahead! :)

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Oct 1, 2018

@rudolfwalter Nope you're good, thanks for getting this started! :)

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Oct 1, 2018

Closing in favor of #54. Thanks @rudolfwalter!

@dwnusbaum dwnusbaum closed this Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.