Skip to content

8354679: [CRaC] jdk.crac.management makes JdkManagementCheckSince fail #225

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

Open
wants to merge 1 commit into
base: crac
Choose a base branch
from

Conversation

TimPushkin
Copy link
Collaborator

@TimPushkin TimPushkin commented Apr 16, 2025

Fixes the failing test, for simplicity pretending that both jdk.crac and jdk.management/jdk.crac.management were added in JDK 24 and before that there was no CRaC in the JDK. Otherwise we would need to retroactively generate symbols for JDKs 17–23 which is a decent amount of work (there are no public CRaC builds for some of these versions).

JDK 24 symbols were updated this way:

  1. Create a custom build from the last OpenJDK 24 CRaC commit 884d074 (the last commit before JDK 25 was merged).
  2. Update the symbols from that build using make/scripts/generate-symbol-data.sh.
  3. Manually remove the CRaC methods removed in d64fb30 from the symbols.

Also adds the since-checking tests to CI.

I initially wanted to also add a since-checking test for jdk.crac module but SinceChecker seems to have a bug which makes the test fail with “module: jdk.crac: @since version is 24 but the element exists before JDK 10”. I believe this is a SinceChecker bug because the same happens for other modules added after JDK 9 without a legacy preview, e.g. jdk.graal.compiler.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8354679: [CRaC] jdk.crac.management makes JdkManagementCheckSince fail (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/crac.git pull/225/head:pull/225
$ git checkout pull/225

Update a local copy of the PR:
$ git checkout pull/225
$ git pull https://git.openjdk.org/crac.git pull/225/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 225

View PR using the GUI difftool:
$ git pr show -t 225

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/225.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 16, 2025

👋 Welcome back tpushkin! A progress list of the required criteria for merging this PR into crac will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Apr 16, 2025

@TimPushkin This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8354679: [CRaC] jdk.crac.management makes JdkManagementCheckSince fail

Reviewed-by: rvansa

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the crac branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@rvansa) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 16, 2025
@mlbridge
Copy link

mlbridge bot commented Apr 16, 2025

Webrevs

@rvansa
Copy link
Collaborator

rvansa commented Apr 17, 2025

I think that it's OK to add the symbols to 24 only; CRaC versioning can be considered orthogonal to upstream JDK versioning. At this stage we keep backward compatibility for users' convenience, not as a rule. Testing API compatibility is useful as CRaC'ed JDK should be 100% usable as a drop-in replacement for upstream JDKs.

If jdk.crac can't be addressed, we can leave it out of scope. However could you file a bug for SinceChecker in upstream?

Last but not least; please outline the steps (can be just a comment on this PR) what will we have to do when we rebase on top of JDK 26.

@TimPushkin
Copy link
Collaborator Author

However could you file a bug for SinceChecker in upstream?

Filed the SinceChecker bug as JDK-8354921.

Last but not least; please outline the steps (can be just a comment on this PR) what will we have to do when we rebase on top of JDK 26.

Whenever we merge an update of the symbols from the mainline (can happen multiple times between a rampdown and the respective GA) we'll need to first overwrite our symbols with the incoming changes and then run make/scripts/generate-symbol-data.sh to update the symbols with CRaC's ones.

Whenever we add a method to the public CRaC Java API we also need to run the script to update the symbols.

I also found JDK-8345212, which we don't yet have in this fork. It should allow us to continue using @since TBD (or @since CRaC, or whatever) and the test will pretend like the CRaC API has been added in the current version — we won't need to do the symbols updates. @rvansa WDYT?

@rvansa
Copy link
Collaborator

rvansa commented Apr 22, 2025

Whenever we merge an update of the symbols from the mainline (can happen multiple times between a rampdown and the respective GA) we'll need to first overwrite our symbols with the incoming changes and then run make/scripts/generate-symbol-data.sh to update the symbols with CRaC's ones.

OK, I thought that we could somehow preserve the oldest CRaC JDK version where the symbol appeared. But we don't need to change the taglet on too many places, so let's use this as is.

I also found JDK-8345212, which we don't yet have in this fork. It should allow us to continue using @SInCE TBD (or @SInCE CRaC, or whatever) and the test will pretend like the CRaC API has been added in the current version — we won't need to do the symbols updates.

That sounds useful, but we don't need to merge changes out-of-band, this PR is good as it is. Let's integrate this and use the -ignoreSince the next time this code needs updating anyway.

@TimPushkin
Copy link
Collaborator Author

OK, I thought that we could somehow preserve the oldest CRaC JDK version where the symbol appeared. But we don't need to change the taglet on too many places, so let's use this as is.

Symbols are fixed after GA so after GA they'll be preserved. But between rampdown and GA we'll need to be updating them (only the version that has not yet been GA-ed) when they are updated upstream.

Example for JDK 26:

  1. 25 rampdown happens, 26 is created and symbols for 25 along with it — when merging this, we'll take the new 25 symbols and run the script to update them from the last 25-CRaC build.
  2. 25's symbols get updated — when merging this, we'll overwrite our 25's symbols with the incoming ones, then run the script again to update them with the last 25-CRaC build (the same build as in step one), then fixup the symbols manually to remove the changes unrelated to CRaC (there will be such changes because the last 25-CRaC build will be based on an older 25 than the updated symbols). This step can happen a few times.
  3. 25 GA happens — after this moment 25's symbols are fixed, we won't need to touch them ever again.

That sounds useful, but we don't need to merge changes out-of-band, this PR is good as it is. Let's integrate this and use the -ignoreSince the next time this code needs updating anyway.

I was proposing to wait until that change gets merged-in. I believe we need to decide now: we either merge this PR and start generating CRaC symbols from now on or wait for the SinceChecker update that should allow us to continue without generating the symbols (I actually need to try this locally by cherry-picking to see how it works before we decide). It will be weird if now we start generating the symbols but then we suddenly stop.

@TimPushkin
Copy link
Collaborator Author

Oh, actually, I believe we are still merging 25 updates that occurred before 24 GA, so we'll go through the steps 2 and 3 above even for 24's symbols.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Pull request is ready to be integrated rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

2 participants