Add a serialVersionUID to hudson.EnvVars to avoid deserialisation errors

[markawm]

See JENKINS-56161.

Following some changes to EnvVars class we've seen some deserialisation errors due to the
auto-generated serialVersionUID - adding an explicit serialVersionUID to avoid these.

No new tests added as this is just adding a serialVersionUID to a class.

Proposed changelog entries

• Internal: Avoid deserialisation errors with EnvVars by adding a stable serialVersionUID.

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

[daniel-beck]

Could you be more specific about the errors you're seeing? Please reference a Jira issue describing the problem.

[markawm]

Yeah, sure. The error I'm getting is:
java.io.InvalidClassException: hudson.EnvVars; local class incompatible: stream classdesc serialVersionUID = -234198271843838088, local class serialVersionUID = 4320331661987259022
This seen when CJOC tries to run a remote operation because the Serializable EnvVars class doesn't define a stable serialVersionUID - but of course could equally affect any other plugin doing the same thing.

I haven't created an issue in Jira based on the guidance in the PR template "We do not require JIRA issues for minor improvements." - but happy to do so if you would prefer one.

[alecharp]

With a Jira it would be easier for other user to see if the issue is known / fixed in the future. I personally tend to search in Jira for known issue.

I think it wouldn't cost a lot of time to create a ticket in Jira, assign it to yourself and link it with this PR. That'd be great. Thanks.

[markawm]

No problem, filed here: https://issues.jenkins-ci.org/browse/JENKINS-56161

[alecharp]

Thank you. Let's discuss the issue on the ticket, we will discuss the code here.

[amuniz]

Regardless where or how the issue is happening this is fixing a bad practice, since a serializable class is not defining a serialVersionUID.

From java.io.Serializable javadocs: it is strongly recommended that all serializable classes explicitly declare serialVersionUID values.

Why does this trivial one-liner change is generating such a friction? As long as the value is the right one (coming from the previous class revision) then this looks good to me.

[alecharp]

There is absolutely no friction here. Just trying to understand why it is now something we need and how it can help everyone.

[daniel-beck]

I wondered at first why not go with a trivial value, but it makes sense to not introduce further deserialization problems by just using the same value as would be generated. So LGTM.

[daniel-beck]

Why does this trivial one-liner change is generating such a friction?

I'm sorry you perceive a request to clarify the purpose of the change, and to file a Jira issue, as (unnecessary) friction. Perhaps it helps if I explain why. There are several good reasons we ask for such things:

For example, we've seen proposed changes in the past that did not actually fix the problem they claimed to fix. When no problem description exists (~ no bug report), it's impossible to determine whether the change accomplishes the stated goal.

We try to write changelogs useful to users who are not Jenkins contributors. Why a change is useful is often not obvious from what it does. For example, knowing the former let's us reference affected plugins when we fix bugs in core causing problems elsewhere, see e.g. the two mentions of Timestamper on https://jenkins.io/changelog/.

Additionally, having a Jira issue is a prerequisite to get things backported into LTS, which makes sense for many bug fixes and similar changes, especially if they're as straightforward as this one. This change could make it into 2.164.1 already.

[markawm]

Thanks, Daniel, Alan. Yes, Daniel - reason for not going with a trivial value was exactly as you describe.

[alecharp]

As said, this LGTM.

[batmat]

Note that despite I fully agree this is a good idea to use the value that's generated by default in the errors you've seen, FWIW IIRC the generated value is not defined in the spec, i.e. it could still fail on various JDKs implementations.

But anyway, we cannot do better than that, and at least this will make it fixed for all implementations in the future so 👍.

Also, if we go this path, should we then review all Serializable classes in Jenkins Core? Before this issue arises on other classes?

Looking https://github.com/jenkinsci/jenkins/search?q=Serializable&unscoped_q=Serializable, it seems we would ideally need to patch a lot of others isn't it?

[markawm]

"the generated value is not defined in the spec"
Interesting, I wasn't aware of that.

"should we then review all Serializable classes"
I guess yes, ideally. Though its probably even more than that list as in this case the class only implements Serializable indirectly.