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-48116] - Restore AbstractTaskListener binary compatibility in the core. #3154

Merged
merged 1 commit into from Nov 26, 2017

Conversation

6 participants
@oleg-nenashev
Member

oleg-nenashev commented Nov 21, 2017

Reverts some bits from #3122 by @jglick.

Since we have the confirmed regression due to the binary compatibility change, I think we need to restore the compatibility. OTOH, I restricted the class, so all users will be forced to stop using it when they upgrade the core.

See JENKINS-48116.

No tests required, it does not change the behavior of the core itself

Proposed changelog entries

  • Entry 1: Restore binary compatibility for external AbstractTaskListener usages (regression in 2.91, impacts Ruby Runtime Plugin and its dependencies).

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees @jglick @daniel-beck

[JENKINS-48116] - Restrore AbstractTaskListener binary compatibility …
…in the core.

Since we have the confirmed regression due to the binary compatibility change, I think we need to restore the compatibility.
OTOH, I restricted the class, so all users will be forced to stop using it when they updgrade the core.

@oleg-nenashev oleg-nenashev changed the title from [JENKINS-48116] - Restrore AbstractTaskListener binary compatibility in the core. to [JENKINS-48116] - Restore AbstractTaskListener binary compatibility in the core. Nov 21, 2017

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Nov 21, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

reviewbybees commented Nov 21, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Nov 21, 2017

Member

This is a clear bug in ruby-runtime that expects all TaskListener to be AbstractTaskListener. It's not in the API the plugins get, so it's a faulty assumption.

Not against doing this minor change to restore compatibility for now, but let's be clear about where the problem lies. It's not with core.

Member

daniel-beck commented Nov 21, 2017

This is a clear bug in ruby-runtime that expects all TaskListener to be AbstractTaskListener. It's not in the API the plugins get, so it's a faulty assumption.

Not against doing this minor change to restore compatibility for now, but let's be clear about where the problem lies. It's not with core.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 21, 2017

Member

@daniel-beck Yep, I agree Ruby runtime still needs to be fixed ("Ruby Runtime plugin still needs to be fixed IMHO, but a hotfix for the core seems to be reasonable" in the ticket). It does not mean I want to touch that plugin

Member

oleg-nenashev commented Nov 21, 2017

@daniel-beck Yep, I agree Ruby runtime still needs to be fixed ("Ruby Runtime plugin still needs to be fixed IMHO, but a hotfix for the core seems to be reasonable" in the ticket). It does not mean I want to touch that plugin

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Nov 21, 2017

Member

It is not obvious to me that this PR would actually fix the reported error. Did you confirm the fix?

Member

jglick commented Nov 21, 2017

It is not obvious to me that this PR would actually fix the reported error. Did you confirm the fix?

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 21, 2017

Member

It is not obvious to me that this PR would actually fix the reported error. Did you confirm the fix?

nope

Member

oleg-nenashev commented Nov 21, 2017

It is not obvious to me that this PR would actually fix the reported error. Did you confirm the fix?

nope

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick Nov 21, 2017

Member

Seems to work, but needs automated test coverage. Working on it.

Member

jglick commented Nov 21, 2017

Seems to work, but needs automated test coverage. Working on it.

@jglick

jglick approved these changes Nov 21, 2017

/**
* @deprecated implement {@link TaskListener} directly
*/
@Deprecated
@Restricted(NoExternalUse.class)

This comment has been minimized.

@jglick

jglick Nov 21, 2017

Member

Overkill. Was already deprecated.

@jglick

jglick Nov 21, 2017

Member

Overkill. Was already deprecated.

This comment has been minimized.

@svanoort

svanoort Nov 22, 2017

Member

People tend to ignore deprecation warnings though.

@svanoort

svanoort Nov 22, 2017

Member

People tend to ignore deprecation warnings though.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 22, 2017

Member

If we want to kill it, I see no problem with overkilling

@oleg-nenashev

oleg-nenashev Nov 22, 2017

Member

If we want to kill it, I see no problem with overkilling

This comment has been minimized.

@jtnord

jtnord Nov 28, 2017

Member

problem is that killing it means you need to use the TaskListener.
TaskListener did not have the methods in prior versions.

Pipelines checking compatability with current and newest cores will break here and there is no easy solution. This makes me a sad panda.

Deprecated code should not be restricted - it should be deprecated, then later restricted then later removed IMO. Needs a JEP.

@jtnord

jtnord Nov 28, 2017

Member

problem is that killing it means you need to use the TaskListener.
TaskListener did not have the methods in prior versions.

Pipelines checking compatability with current and newest cores will break here and there is no easy solution. This makes me a sad panda.

Deprecated code should not be restricted - it should be deprecated, then later restricted then later removed IMO. Needs a JEP.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 28, 2017

Member

Maybe. I'd guess it makes sense to create a revert PR

@oleg-nenashev

oleg-nenashev Nov 28, 2017

Member

Maybe. I'd guess it makes sense to create a revert PR

This comment has been minimized.

@jglick

jglick Nov 28, 2017

Member

Pipelines checking compatability with current and newest cores will break here

If there were any such. plugin-compat-tester ignores source compatibility.

Anyway

Deprecated code should not be restricted - it should be deprecated

Yes. If people ignore deprecation warnings, and we have serious deprecations we really need people to stop using by a particular time, we should be addressing that in buildPlugin.groovy, or plugin-pom, or grepping for usages, or searching for PCT failures, or something. But

then later restricted then later removed

Or simply removed. There is no need for @Restricted in this context at all. There is only one purpose for @Restricted: introducing a member which for technical reasons must be public (for example, due to reflective calls) but which you do not actually want or expect to be referenced statically. It is a workaround for the lack of a module system in Java 8-. It has nothing to do with deprecation. IMO.

@jglick

jglick Nov 28, 2017

Member

Pipelines checking compatability with current and newest cores will break here

If there were any such. plugin-compat-tester ignores source compatibility.

Anyway

Deprecated code should not be restricted - it should be deprecated

Yes. If people ignore deprecation warnings, and we have serious deprecations we really need people to stop using by a particular time, we should be addressing that in buildPlugin.groovy, or plugin-pom, or grepping for usages, or searching for PCT failures, or something. But

then later restricted then later removed

Or simply removed. There is no need for @Restricted in this context at all. There is only one purpose for @Restricted: introducing a member which for technical reasons must be public (for example, due to reflective calls) but which you do not actually want or expect to be referenced statically. It is a workaround for the lack of a module system in Java 8-. It has nothing to do with deprecation. IMO.

This comment has been minimized.

@jtnord

jtnord Nov 28, 2017

Member

@jglick

Pipelines checking compatibility with current and newest cores will break here

If there were any such. plugin-compat-tester ignores source compatibility.

They exist, they are just not open source. PCT is checking binary compatibility. This closed source one checks source compatibility as there is a need to check that as well.

Whilst sometimes source compatibility is broken for a reasonable reason - in this case it seems a bit pointless, as there is no way to be source compatible with the earlier LTS and later weekly versions.

then later restricted then later removed

Or simply removed.

Something we can discuss in the future JEP I will be writing :-)

@jtnord

jtnord Nov 28, 2017

Member

@jglick

Pipelines checking compatibility with current and newest cores will break here

If there were any such. plugin-compat-tester ignores source compatibility.

They exist, they are just not open source. PCT is checking binary compatibility. This closed source one checks source compatibility as there is a need to check that as well.

Whilst sometimes source compatibility is broken for a reasonable reason - in this case it seems a bit pointless, as there is no way to be source compatible with the earlier LTS and later weekly versions.

then later restricted then later removed

Or simply removed.

Something we can discuss in the future JEP I will be writing :-)

This comment has been minimized.

@jglick

jglick Jan 23, 2018

Member

FTR this just reared its ugly head again. maven-plugin cannot be compiled against weeklies because of this. And we cannot change its code to just implement TaskListener, because then it will not work with its original baseline. So we need to copy the hyperlink implementation. Irritating problem of our own making.

@jglick

jglick Jan 23, 2018

Member

FTR this just reared its ugly head again. maven-plugin cannot be compiled against weeklies because of this. And we cannot change its code to just implement TaskListener, because then it will not work with its original baseline. So we need to copy the hyperlink implementation. Irritating problem of our own making.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jan 23, 2018

Member

@jglick could you propose a PR restoring it?

@oleg-nenashev

oleg-nenashev Jan 23, 2018

Member

@jglick could you propose a PR restoring it?

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 21, 2017

Member

@reviewbybees done
Thanks for the autotest, @jglick !

Member

oleg-nenashev commented Nov 21, 2017

@reviewbybees done
Thanks for the autotest, @jglick !

@svanoort

Approved once tests go green.

@oleg-nenashev oleg-nenashev merged commit f33506f into jenkinsci:master Nov 26, 2017

1 check failed

continuous-integration/jenkins/pr-head This commit has test failures
Details

@jglick jglick referenced this pull request Jan 23, 2018

Merged

[JENKINS-49089] Whitelist problems #112

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment