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

[FIXED JENKINS-20559 and JENKINS-19454] - hudson.Launcher bugfix and additional wrapper #1013

Merged
merged 3 commits into from Jun 3, 2014

Conversation

oleg-nenashev
Copy link
Member

Summary:

  • A fix for NPE in hudson.Launcher.ProcSrater::envs()
  • A DecoratedLauncher code, which is being duplicated by many plugins (Custom Tools, etc.)
  • A test for JENKINS-20559, which uses a DecoratedLauncher

Noting:
<plaintext>

  • Uninitialized Process Starter environment may cause NPE in build wrappers (issue 20559)
  • New API class: DecoratedLauncher, which delegated all call to nested launcher (issue 19454) </plaintext> The last bullet is a reason, why these changes have been merged into a single PR
  • @cloudbees-pull-request-builder

    core » jenkins_main_trunk #1538 UNSTABLE
    Looks like there's a problem with this pull request

    @kutzi
    Copy link
    Member

    kutzi commented Nov 14, 2013

    Probably better to have this in 2 different PRs.
    The NPE fix seems like something very uncontentious, which could be merged immediately

    @oleg-nenashev
    Copy link
    Member Author

    I can submit the fix without tests as a separate PR.

    @oleg-nenashev
    Copy link
    Member Author

    @kutzi
    BTW, I'm not sure about returning a null. Maybe, an empty array would be preferable

    @kutzi
    Copy link
    Member

    kutzi commented Nov 16, 2013

    Yes, agreed. Empty array is preferable over null

    @cloudbees-pull-request-builder

    core » jenkins_main_trunk #1561 SUCCESS
    This pull request looks good

    @cloudbees-pull-request-builder

    core » jenkins_main_trunk #1567 FAILURE
    Looks like there's a problem with this pull request

    @cloudbees-pull-request-builder

    core » jenkins_main_trunk #1568 UNSTABLE
    Looks like there's a problem with this pull request

    @oleg-nenashev
    Copy link
    Member Author

    hudson.util.AlternativeUiTextProviderTest.testBasics returns "java.lang.IllegalStateException: second instance".
    I see the same issue in #1022

    Seems yi has been caused by previous commits...

    @jglick
    Copy link
    Member

    jglick commented Nov 18, 2013

    Yes I think that was @kohsuke’s regression.

    @oleg-nenashev
    Copy link
    Member Author

    BTW, the PR is ready for further review and merge

    @oleg-nenashev
    Copy link
    Member Author

    Hello,
    Should I do anything else before the merge?
    Seems that this PR hangs.

    @oleg-nenashev
    Copy link
    Member Author

    @olivergondza or @jglick
    Could you review the PR?
    synopsys-arc-oss@49a1838 and synopsys-arc-oss@73cb186 fix a very annoying bug, which appears in the case of multiple build wrapper plugins, so it makes sense to merge it into 1.532.x LTS

    * @author Oleg Nenashev <nenashev@synopsys.com>, Synopsys Inc.
    * @since TODO: define a version
    */
    public class ProcStarterTest extends JenkinsRule {
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Extending JenkinsRule is rather unusual. Also @since for TestCase is unnecessary.

    @jglick
    Copy link
    Member

    jglick commented Dec 23, 2013

    Right. Otherwise the PR looks OK to me.

    @oleg-nenashev
    Copy link
    Member Author

    Thanks a lot.
    I'll fix the test-case and then squash commits.

    oleg-nenashev added a commit to synopsys-arc-oss/jenkins that referenced this pull request Jan 2, 2014
    Signed-off-by: Oleg Nenashev <nenashev@synopsys.com>
    @oleg-nenashev
    Copy link
    Member Author

    I've merged changes into several commits. A direct test for JENKINS-20559 goes in a seoarate commit in order to simplify LTS merge of synopsys-arc-oss@320a26d

    Unfortunately, seems that https://jenkins.ci.cloudbees.com/job/core/job/jenkins_main_trunk/ has gone to New Year holidays :(

    @jglick
    Copy link
    Member

    jglick commented Jan 2, 2014

    Try merging with master (or rebasing) and pushing again. I think I fixed PR building.

    @cloudbees-pull-request-builder

    core » jenkins-core #4 UNSTABLE
    Looks like there's a problem with this pull request

    @cloudbees-pull-request-builder

    core » jenkins-core #6 UNSTABLE
    Looks like there's a problem with this pull request

    @cloudbees-pull-request-builder

    core » jenkins-core #304 UNSTABLE
    Looks like there's a problem with this pull request

    … core.
    
    This launcher Allows subclasses to only implement methods they want to override.
    Originally, this launcher has been implemented in Custom Tools Plugin, but there are many duplicates in other plugins => it would be useful to have it in Jenkins core.
    
    Resolves https://issues.jenkins-ci.org/browse/JENKINS-19454
    
    Signed-off-by: Oleg Nenashev <nenashev@synopsys.com>
    … environment variables list
    
    Resolves https://issues.jenkins-ci.org/browse/JENKINS-20559
    BTW, I also need to add some tests to "Test Harness" in order to improve coverage of nested operations.
    These test require a fix for https://issues.jenkins-ci.org/browse/JENKINS-19454
    
    Signed-off-by: Oleg Nenashev <nenashev@synopsys.com>
    @cloudbees-pull-request-builder

    core » jenkins-core #789 UNSTABLE
    Looks like there's a problem with this pull request

    @cloudbees-pull-request-builder

    core » jenkins-core #797 FAILURE
    Looks like there's a problem with this pull request

    @cloudbees-pull-request-builder

    core » jenkins-core #800 FAILURE
    Looks like there's a problem with this pull request

    Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
    @cloudbees-pull-request-builder

    core » jenkins-core #801 SUCCESS
    This pull request looks good

    @oleg-nenashev
    Copy link
    Member Author

    I've re-based changes and fixed the unit test according to comments.
    The PR is ready to be merged

    oleg-nenashev added a commit that referenced this pull request Jun 3, 2014
    [FIXED JENKINS-20559 and JENKINS-19454] - hudson.Launcher bugfix and additional wrapper
    @oleg-nenashev oleg-nenashev merged commit 4d4ca20 into jenkinsci:master Jun 3, 2014
    oleg-nenashev added a commit that referenced this pull request Jun 3, 2014
    Signed-off-by: Oleg Nenashev <o.v.nenashev@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    5 participants