helpdesk icon indicating copy to clipboard operation
helpdesk copied to clipboard

incremental and release versions are different

Open jtnord opened this issue 2 years ago • 16 comments

Service(s)

Incrementals (probably more CD JEP-229)

Summary

the incremental build is not the same as the release build - causes random build failures.

it was my expectation that an incremental build would be copied - so it was identical to the release build - but this is not the case. depsite having the same release version the digests of the files are different implying they have been fully rebuilt.

This causes issues for developers and any other system that has the expectation that a release version is golden and will never change.

For example https://github.com/jenkinsci/acceptance-test-harness/issues/1297 if you have had incrementals installed and then run the ATH the shas will be for the plugin in the release repo which has a different digest.

There are various other issues with reusing released versions also.

Reproduction steps

create a PR to get an incremental build merge the PR to get a release build

compare the digest of the 2 hpis in artifactory

expectation is that they are identical, however they are not.

e.g. compare https://repo.jenkins-ci.org/ui/repos/tree/General/releases/org/jenkins-ci/plugins/jackson2-api/2.15.2-350.v0c2f3f8fc595/jackson2-api-2.15.2-350.v0c2f3f8fc595.hpi vs https://repo.jenkins-ci.org/ui/repos/tree/General/incrementals/org/jenkins-ci/plugins/jackson2-api/2.15.2-350.v0c2f3f8fc595/jackson2-api-2.15.2-350.v0c2f3f8fc595.hpi

the shas are different c8ca30e9759c473a7bf8d21f29f3f291be0030f183bfd173adb06b8c555b6d82 (incremental repo) vs 41c78ad46a4ce3fc40b353bf4cd425aa18d2a4dbfe826624c62834e4b19fdaad (non incremental)

jtnord avatar Jul 27 '23 09:07 jtnord

as the hashes are different the releases really need to be a different version - which removes some benifit of JEP-229 however as some people use "merge squashing" (without need) of PRs we have this situation for several plugins today. either that or we need to ensure that we have fully build reproducability in the plugin pom so that you can create a bitwise identical version from the same sources.

jtnord avatar Jul 27 '23 09:07 jtnord

is it just a matter of putting a -rc in the changelist ?

timja avatar Jul 27 '23 09:07 timja

cc @jglick

timja avatar Jul 27 '23 09:07 timja

is it just a matter of putting a -rc in the changelist ?

that could certainly solve the issue.

It would IIUC break @jglick's workflow though as he often builds up PRs of incrementals and can then just merge them in order without updating them to pickup the "released" version.

But I have long argued that the incremental version should be clearly identified as not a release - as they can be (although they have never been to this date) removed at any time, and may not even end up becoming a release; they are really a just a SNAPSHOT in all but version, and cannot easily be identified as such..

jtnord avatar Jul 27 '23 09:07 jtnord

the digests of the files are different implying they have been fully rebuilt

They have been, on GHA. This is a deliberate design choice in JEP-229 (ci.jenkins.io is considered a developer tool rather than a trusted source of release binaries).

create a PR to get an incremental build merge the PR to get a release build

Did you really do exactly these steps? Because then of course they would be different—they would have different versions. The first would be of your PR head, the second would be a merge commit (or rebase or squash) into trunk.

If you really git merge --fast-forward from the CLI to merge the PR (there is no equivalent from the web GUI) then you should have the same version. Whether the checksums of those artifacts are the same would depend on whether or not we have set up reproducible builds of Jenkins plugins. Last I checked, we had not, though there had been some movement in that direction.

Normally a given version would be deployed to either incrementals or releases but not both. https://github.com/jenkinsci/jackson2-api-plugin/commit/0c2f3f8fc5958f06227af89b438134c09b4829d9 was a merge commit of https://github.com/jenkinsci/jackson2-api-plugin/pull/202 by @alecharp. I suspect it is the use of the merge queue which led to this behavior in this example: that commit was first tested in trunk by the queue build, then pushed as is (and so released). We could probably adjust buildPlugin to detect that the $BRANCH_NAME is that of a merge queue (since they follow a certain pattern) and suppress the incrementals publisher, since publishing from here would be at best useless. Actually it would probably suffice to only publish from PR-*, i.e., when CHANGE_ID is set.

(It would also be possible to enable incrementals publishing only on PRs marked to have that done with a label. This would restrict publishing to authors who either have triage permission on the repo, or can find a maintainer to add that label for them. Or triggered by PR title, etc. Then you would opt into publishing only for the minority of PRs which are actually intended as upstreams of some other PR or process.)

jglick avatar Jul 27 '23 11:07 jglick

Did you really do exactly these steps? Because then of course they would be different—they would have different versions. The first would be of your PR head, the second would be a merge commit (or rebase or squash) into trunk.

I'm just sumizing - we have identical versions in incrementals and release repository. I guess then someone is squash merging a PR with a single commit or fast forward merging so the versions are identical.

I do not do this at all, and generally always merge (not rebase or squash) from the GH UI.

Normally a given version would be deployed to either incrementals or releases but not both. https://github.com/jenkinsci/jackson2-api-plugin/commit/0c2f3f8fc5958f06227af89b438134c09b4829d9 was a merge commit of https://github.com/jenkinsci/jackson2-api-plugin/pull/202 by @alecharp. I suspect it is the use of the merge queue which led to this behavior in this example: that commit was first tested in trunk by the queue build, then pushed as is (and so released

also seen on mina-sshd plugins -> https://github.com/jenkinsci/mina-sshd-api-plugin/pull/64

it is pretty easy to see on the ATH as installing plugins fails as shas do not match (although pulling out that info is not so easy). As ATH is "incrementalified" the incrementals repo is pulled before public Why this is not reliably failing in CI I do not yet know - perhaps the AHC is checking public before releases (which means CI builds behave differently to local?) - although depending on how you cache 404 this could cause infra issues - but that should be fixed at source if that is indeed an issue.

jtnord avatar Jul 27 '23 12:07 jtnord

squash merging a PR with a single commit

This will produce a distinct commit AFAIK, so the issue is probably limited to use of the merge queue.

also seen on mina-sshd plugins -> https://github.com/jenkinsci/mina-sshd-api-plugin/pull/64

Not sure about that case: https://github.com/jenkinsci/mina-sshd-api-plugin/commit/28e3e36d18eb71e04e433296ef354dde4b2d8c58 is a merge commit.

jglick avatar Jul 27 '23 12:07 jglick

https://repo.jenkins-ci.org/ui/artifactSearchResults?repositories%5B0%5D=releases&repositories%5B1%5D=incrementals&name=mina-sshd-api-common&type=artifacts

image

workflow-cps-global-lib

image

jtnord avatar Jul 27 '23 12:07 jtnord

Oh I see, this would be coming from a master branch project build. I think incremental publishing can be disabled on non-PR builds; it is occasionally useful to get an incremental build from trunk of a non-CD-enabled component, but at worst you can open a draft PR with an empty commit as a placeholder.

jglick avatar Jul 27 '23 12:07 jglick

Oh I see, this would be coming from a master branch project build. I think incremental publishing can be disabled on non-PR builds; it is occasionally useful to get an incremental build from trunk of a non-CD-enabled component, but at worst you can open a draft PR with an empty commit as a placeholder.

It's useful on core to get from master branch so would be good to not globally disable but agree in general it can be

timja avatar Jul 27 '23 13:07 timja

Should be addressed going forward; not sure if you need this reopened to clean up historical duplicates between incrementals and releases @jtnord?

jglick avatar Jul 27 '23 16:07 jglick

Oh I see, this would be coming from a master branch project build

Why are these not also -rc? Seems to me that incrementals should never build non -rc.

daniel-beck avatar Jul 28 '23 08:07 daniel-beck

Should be addressed going forward

many thanks Jesse

Should be addressed going forward; not sure if you need this reopened to clean up historical duplicates between incrementals and releases @jtnord?

I think this is just an issue for users and not for CI

For the majority of users I doubt it matters if you get an incremental over a release for the same version and the maven metadata for the hashes will be correct for each "duplicate" artefact.

So this probably only affects the ATH where the hashes come from the update center which looks at the releases repo, and for the jenkins war (if it embeds an incremental in a detached plugin that should be a release - but that should only happen for incremental builds - so unclear if this is actually an issue).

let me see if I can get a list of all the duplicates, currently can be worked around locally by removing all artifacts obtained from the incrementals repo with grep -r -l --include=_remote.repositories ">incrementals=" | xargs --max-lines dirname | xargs --max-lines rm -fr (in ~/.m2/repository)

jtnord avatar Jul 28 '23 09:07 jtnord

@daniel-beck helped me out (there is no nexus index for the incrementals repo) and has produced this list

I think we should clean these up - as a minimum cleaning the most recent would probably fix 99% of any possible local ATH issue (the 1% being where the latest release is not compatable with the LTS version)

That said - i have no idea if this would be trivial or a PITA task for someone - if it is trivial I would strongly recommend that they be deleted.

FWIW I can not re-open this ticket.

jtnord avatar Jul 28 '23 11:07 jtnord

this would be coming from a master branch project build

Why are these not also -rc?

For a JEP-229 repo, the changelist.format is overridden to not use -rc: there is no difference between an RC and a release. https://github.com/jenkins-infra/pipeline-library/pull/705 just avoids the redundant build of the same commit with the same version number.

(It also avoids publishing an incremental version from a trunk commit after an “uninteresting” PR is merged, which is presumably fine since if it was not interesting as a dependency then no one should need to consume it; if they did, use the developer label at least.)

I think this is just an issue for users and not for CI

Perhaps you meant the reverse? “Users” would be using the update center, which would be drawing from releases, so the content of incrementals would be irrelevant.

jglick avatar Jul 28 '23 13:07 jglick

I was meaning users as developers rather than end users of Jenkins.

CI jobs work because the ATH does not download incrementals only plugins from /public and will always have a clean local repo at the start of the run so could not have an incremental existing.

Whilst plugins builds can download incremental during a build and thus could behave differently the only difference should§ be some timestamps and checksums which should not be an issue to plugin PRs. - or the release build fails in GitHub as it relies on an incremental dependency even though the branch build passed as it has access to these.

End Users are (hopefully) unaffected - so long as the release build of Jenkins only consumes the non incremental version of any detached plugin.

SO the biggest impact is those of us who build plugins or otherwise have dependencies on plugins that can come from incremental and then be stored in the local maven repo - which will then be used in preference to the artefact from releases. (presumably for anything not just ATHs)

§ should - however it is still bad.

jtnord avatar Jul 31 '23 10:07 jtnord