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

[Build] Use last successful ECJ snapshot in verification builds #2538

Merged

Conversation

HannesWell
Copy link
Member

Use the latest ECJ snapshot version from the last successful I-build (without comparator errors) for verification builds, to automatically align with I-builds.
Currently ECJ has to be deployed manually to a JDT snapshot-repository after changes in ECJ that caused comparator-errors:

This prevents issues like

If there are no other usages of https://repo.eclipse.org/content/repositories/eclipse-staging, this should make the JDT Jenkins job to deploy ECJ to it obsolete

as well as the target repository and should allow us to delete:
https://repo.eclipse.org/content/repositories/eclipse-staging

@jarthana and @akurtakov this is what we talked about in today's PMC call.
@MohananRahul can you please check if the release process documentation update is correct and fine?

@MohananRahul
Copy link
Contributor

@MohananRahul can you please check if the release process documentation update is correct and fine?

Ok , looks good.

Copy link
Member

@akurtakov akurtakov left a comment

Choose a reason for hiding this comment

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

Looks good to me but let's land it after 4.34 is branched to prevent possible last minute surprise.

@HannesWell HannesWell force-pushed the ecj-snapshot-in-iBuilds branch from db61025 to 2524e67 Compare November 14, 2024 21:43
@HannesWell
Copy link
Member Author

HannesWell commented Nov 14, 2024

Looks good to me but let's land it after 4.34 is branched to prevent possible last minute surprise.

Acknowledge.

I have now changed this PR to use a version range for cbi-ecj-version:
<cbi-ecj-version>[3.40.0,4)</cbi-ecj-version>

This way the version does not have to be updated at all as long as ECJ stays at version 4.x. I tried this out in eclipse-pde/eclipse.pde#1467 and it worked as expected.
If we use this, we can simplify the release procedure and just drop the update step.
WDYT?

@laeubi
Copy link
Contributor

laeubi commented Nov 15, 2024

Maven version ranges can give surprising results, e.g. a version of 4.0.0-alpha1 is included in your range.
It is also bad in terms of reproducibility as older branches will get suddenly newer releases.

@HannesWell HannesWell force-pushed the ecj-snapshot-in-iBuilds branch from 2524e67 to 162f2bb Compare November 18, 2024 19:36
@HannesWell
Copy link
Member Author

Maven version ranges can give surprising results, e.g. a version of 4.0.0-alpha1 is included in your range.

In general for the master branch I assume that we want the latest version anyways, so the upper bound could be empty/infinity.

It is also bad in terms of reproducibility as older branches will get suddenly newer releases.

OK, for older branches this is indeed probably not wanted. I reverted that part.
Overall this is still a significant simplification.

If there are no other usages of https://repo.eclipse.org/content/repositories/eclipse-staging, this should make the JDT Jenkins job to deploy ECJ to it obsolete

* https://ci.eclipse.org/jdt/job/copyAndDeployJDTCompiler/

as well as the target repository and should allow us to delete: https://repo.eclipse.org/content/repositories/eclipse-staging

@jarthana or @akurtakov can you tell if there are other users of that repository? If not I suggest we delete that repository and see if there are any unknown users. And if nobody complains or someone cannot be moved to the maven-repo into that the I-builds deploy, we can continue to delete the Jenkins job too.

@akurtakov
Copy link
Member

In the light of #2595 what prevents this one from merging? I'm not troubled by 4.0.0-alpha at all. There is no way to know whether this staging repo is used by anyone else but being "staging" means I have no concerns about changes in it.

@HannesWell HannesWell force-pushed the ecj-snapshot-in-iBuilds branch from 162f2bb to a21cd80 Compare November 27, 2024 20:23
@HannesWell
Copy link
Member Author

In the light of #2595 what prevents this one from merging? I'm not troubled by 4.0.0-alpha at all.

Nothing specific, I just had to update the version to the current version of org.eclipse.jdt.core.compiler.batch. It's currently 3.40.100 so I assume that it will change to 3.41 at some point in this dev-cycle since @jarthana mentioned it's usually always bump by one minor version?
So yes, using a version range would avoid that. But on the other hand, if we build fixes on maintenance branches we would always use the latest ECJ version and not the one used at the time the branch was forked.
But maybe it would be better anyways to make it part of the release procedure to set the cbi-ecj-version property to the fixed version of the about to be released ECJ, when creating the maintanance branche?
Then we could use an open range for the compiler in active development and always use a matching version for maintenance.

There is no way to know whether this staging repo is used by anyone else but being "staging" means I have no concerns about changes in it.

My suggestion is to remove that repo immediately, after this is submitted, to find out if there are other users (by breaking them and hope thy will complain).

@HannesWell HannesWell force-pushed the ecj-snapshot-in-iBuilds branch from a21cd80 to 24c360f Compare November 28, 2024 18:49
@HannesWell
Copy link
Member Author

But maybe it would be better anyways to make it part of the release procedure to set the cbi-ecj-version property to the fixed version of the about to be released ECJ, when creating the maintanance branche?
Then we could use an open range for the compiler in active development and always use a matching version for maintenance.

I have now implemented that:
Use the very latest version of ECJ with an open version-range and adjust the release procedure to set cbi-ecj-version to the exact single version of ecj that is about to be released when preparing the maintenance branch.
For the future the change done in #2558 should include setting cbi-ecj-version.
@MohananRahul is the changed procedure doc clear or should I add further details?

In general I think this solutions gives us the best from both worlds. No configuration for the master and always up to date compilers and stable builds on maintenance branches.
So if there are no objections I'll submit this tomorrow evening.

@MohananRahul
Copy link
Contributor

In the light of #2595 what prevents this one from merging? I'm not troubled by 4.0.0-alpha at all.

Nothing specific, I just had to update the version to the current version of org.eclipse.jdt.core.compiler.batch. It's currently 3.40.100 so I assume that it will change to 3.41 at some point in this dev-cycle since @jarthana mentioned it's usually always bump by one minor version? So yes, using a version range would avoid that. But on the other hand, if we build fixes on maintenance branches we would always use the latest ECJ version and not the one used at the time the branch was forked. But maybe it would be better anyways to make it part of the release procedure to set the cbi-ecj-version property to the fixed version of the about to be released ECJ, when creating the maintanance branche? Then we could use an open range for the compiler in active development and always use a matching version for maintenance.

There is no way to know whether this staging repo is used by anyone else but being "staging" means I have no concerns about changes in it.

My suggestion is to remove that repo immediately, after this is submitted, to find out if there are other users (by breaking them and hope thy will complain).

Removal of repo will cause failures in maintenance branch builds right?

@HannesWell HannesWell force-pushed the ecj-snapshot-in-iBuilds branch from 24c360f to b592667 Compare November 29, 2024 22:16
@HannesWell
Copy link
Member Author

My suggestion is to remove that repo immediately, after this is submitted, to find out if there are other users (by breaking them and hope thy will complain).

Removal of repo will cause failures in maintenance branch builds right?

I checked the version used there and it would indeed break them, because they use a fully-qualified version and at Maven-Central only unqualified versions are used. I adjusted the release procedure to consider that for future maintenance branches.
But for existing ones we either have to keep the repo or update the branches (if needed).
So I guess it's simpler to just keep the repo for some time. But this also means that this, respectivly the adjustments in release procedure will make future maintenance branches more robust since they rely on one staging repo less.

But since there are no objections against this change, I'll submit this now in order to make the latest ECJ also available for verification builds.

@HannesWell
Copy link
Member Author

But since there are no objections against this change, I'll submit this now in order to make the latest ECJ also available for verification builds.

Ok, the Jenkins build of ECJ itself is objecting. Will look into that tomorrow.

@HannesWell HannesWell force-pushed the ecj-snapshot-in-iBuilds branch from b592667 to 3099a5c Compare December 1, 2024 19:45
Use the latest ECJ snapshot version from the last successful I-build
(without comparator errors) for verification builds, to automatically
align with I-builds.
Currently ECJ has to be deployed manually to a JDT snapshot-repository
after changes in ECJ that caused comparator errors. This change makes
that JDT snapshot repository is obsolete.
@HannesWell HannesWell force-pushed the ecj-snapshot-in-iBuilds branch from 3099a5c to 3f7cac8 Compare December 1, 2024 20:13
@HannesWell
Copy link
Member Author

But since there are no objections against this change, I'll submit this now in order to make the latest ECJ also available for verification builds.

Ok, the Jenkins build of ECJ itself is objecting. Will look into that tomorrow.

I think was because 3.40.0-SNAPSHOT is considered to be smaller than 3.40 in the logic of Maven's version comparator, isn't it?
Because when setting the lower-bound to 3.39 it works.
@laeubi can you confirm that, Maven always uses the latest available version that's within the range?
When running the build locally with -X it said that the tycho-compiler-plugin has org.eclipse.jdt:ecj:jar:3.40.0-SNAPSHOT on it's classpath even though a version 3.39 of ECJ is also available on Maven-central.

So I think this is now as desired and finally ready for submission.

@HannesWell HannesWell merged commit 2779ffb into eclipse-platform:master Dec 2, 2024
4 checks passed
@HannesWell HannesWell deleted the ecj-snapshot-in-iBuilds branch December 2, 2024 19:29
HannesWell added a commit to HannesWell/eclipse.platform.releng.aggregator that referenced this pull request Dec 3, 2024
Keeping that variable fails the I-build because is is not used and not
defined anymore since:
eclipse-platform#2538
HannesWell added a commit that referenced this pull request Dec 3, 2024
Keeping that variable fails the I-build because is is not used and not
defined anymore since:
#2538
@HannesWell
Copy link
Member Author

Since we still have comparator issues in some verification builds we should make sure that these verification builds have --update-snapshots respectively it's short form -U specified.

@HannesWell
Copy link
Member Author

Since we still have comparator issues in some verification builds we should make sure that these verification builds have --update-snapshots respectively it's short form -U specified.

In the shared GH workflow this is set, but only for some Jenkinsfile:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants