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

8285497: Add system property for Java SE specification maintenance version #100

Closed
wants to merge 3 commits into from
Closed

Conversation

jddarcy
Copy link
Member

@jddarcy jddarcy commented Aug 11, 2022

Same diff as used in the JDK 8u RI update.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8285497: Add system property for Java SE specification maintenance version

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev pull/100/head:pull/100
$ git checkout pull/100

Update a local copy of the PR:
$ git checkout pull/100
$ git pull https://git.openjdk.org/jdk8u-dev pull/100/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 100

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/100.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 11, 2022

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

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 11, 2022
@mlbridge
Copy link

mlbridge bot commented Aug 11, 2022

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora 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. Thanks.

@openjdk
Copy link

openjdk bot commented Aug 12, 2022

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

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

8285497: Add system property for Java SE specification maintenance version

Reviewed-by: dholmes, iris, mchung, andrew

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 4 new commits pushed to the master branch:

  • e3251a2: 8232950: SUNPKCS11 Provider incorrectly check key length for PSS Signatures.
  • 41c7d2d: 8039955: [TESTBUG] jdk/lambda/LambdaTranslationTest1 - java.lang.AssertionError: expected [d:1234.000000] but found [d:1234,000000]
  • a4d4973: 8254318: Remove .hgtags
  • 8ad9018: 8285400: Add '@APinote' to the APIs defined in Java SE 8 MR 3

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 12, 2022
Copy link
Member

@irisclark irisclark left a comment

Choose a reason for hiding this comment

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

Spec changes match those in JSR 337 MR 4.

Copy link
Member

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

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

Change looks good and nearly the same as that in 8u42. However, one copyright header change was missed in System.c, presumably due to 8189761 already having bumped it to 2019. Can we fix this please?

Also, please change the title to "Backport 31a63ba5f255e09349b3842984ac5bb3ad8e6c0b" so SKARA correctly identifies this as a backport. See https://wiki.openjdk.org/display/SKARA/Backports

@jddarcy jddarcy changed the title JDK-8285497: Add system property for Java SE specification maintenance version Backport 31a63ba5f255e09349b3842984ac5bb3ad8e6c0b Aug 22, 2022
@openjdk openjdk bot changed the title Backport 31a63ba5f255e09349b3842984ac5bb3ad8e6c0b 8285497: Add system property for Java SE specification maintenance version Aug 22, 2022
@openjdk
Copy link

openjdk bot commented Aug 22, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added the backport label Aug 22, 2022
@jddarcy
Copy link
Member Author

jddarcy commented Aug 22, 2022

Change looks good and nearly the same as that in 8u42. However, one copyright header change was missed in System.c, presumably due to 8189761 already having bumped it to 2019. Can we fix this please?

Also, please change the title to "Backport 31a63ba5f255e09349b3842984ac5bb3ad8e6c0b" so SKARA correctly identifies this as a backport.

I've done as you've requested.

See https://wiki.openjdk.org/display/SKARA/Backports

Yes, I am familiar with the basic operation of Skara. I don't see anything on the referenced page that requires use of a Skara Backport PR when doing a backport. The impetus for adding the backport feature to Skara, a feature enabled several months after JDK mainline switched to Skara, was to reduce the overhead of the common case of backport a fix to an earlier release where few to no changes are needed.

Before sending out this PR, I looked for, but did not find, documentation for what procedures 8u wanted to use for a situation like this.

Copy link
Member

@gnu-andrew gnu-andrew left a comment

Choose a reason for hiding this comment

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

Updated patch looks good

@gnu-andrew
Copy link
Member

gnu-andrew commented Aug 24, 2022

Change looks good and nearly the same as that in 8u42. However, one copyright header change was missed in System.c, presumably due to 8189761 already having bumped it to 2019. Can we fix this please?
Also, please change the title to "Backport 31a63ba5f255e09349b3842984ac5bb3ad8e6c0b" so SKARA correctly identifies this as a backport.

I've done as you've requested.

See https://wiki.openjdk.org/display/SKARA/Backports

Yes, I am familiar with the basic operation of Skara. I don't see anything on the referenced page that requires use of a Skara Backport PR when doing a backport. The impetus for adding the backport feature to Skara, a feature enabled several months after JDK mainline switched to Skara, was to reduce the overhead of the common case of backport a fix to an earlier release where few to no changes are needed.

Before sending out this PR, I looked for, but did not find, documentation for what procedures 8u wanted to use for a situation like this.

Thanks for the feedback. I can improve the documentation on the 8u wiki to make this clearer. If Skara doesn't recognise the issue as a backport, it doesn't seem to like the referenced bug. Prior to you updating the title, it was issuing a warning that the bug was not open (as we'd expect), so I'm not sure it would correctly resolve it with a backport issue when pushed. Using the "Backport <x>" nomenclature just seems to make things work well in all cases.

Thanks for making the changes. I've approved this for 8u now so it's good to integrate.

@gnu-andrew
Copy link
Member

Also, it's not necessary for this PR, but you might want to enable GitHub Actions on your 8u fork for any future PRs.

@jddarcy
Copy link
Member Author

jddarcy commented Aug 24, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Aug 24, 2022

Going to push as commit d0ad242.
Since your change was applied there have been 4 commits pushed to the master branch:

  • e3251a2: 8232950: SUNPKCS11 Provider incorrectly check key length for PSS Signatures.
  • 41c7d2d: 8039955: [TESTBUG] jdk/lambda/LambdaTranslationTest1 - java.lang.AssertionError: expected [d:1234.000000] but found [d:1234,000000]
  • a4d4973: 8254318: Remove .hgtags
  • 8ad9018: 8285400: Add '@APinote' to the APIs defined in Java SE 8 MR 3

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Aug 24, 2022
@openjdk openjdk bot closed this Aug 24, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 24, 2022
@openjdk
Copy link

openjdk bot commented Aug 24, 2022

@jddarcy Pushed as commit d0ad242.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jddarcy
Copy link
Member Author

jddarcy commented Aug 24, 2022

Change looks good and nearly the same as that in 8u42. However, one copyright header change was missed in System.c, presumably due to 8189761 already having bumped it to 2019. Can we fix this please?
Also, please change the title to "Backport 31a63ba5f255e09349b3842984ac5bb3ad8e6c0b" so SKARA correctly identifies this as a backport.

I've done as you've requested.

See https://wiki.openjdk.org/display/SKARA/Backports

Yes, I am familiar with the basic operation of Skara. I don't see anything on the referenced page that requires use of a Skara Backport PR when doing a backport. The impetus for adding the backport feature to Skara, a feature enabled several months after JDK mainline switched to Skara, was to reduce the overhead of the common case of backport a fix to an earlier release where few to no changes are needed.
Before sending out this PR, I looked for, but did not find, documentation for what procedures 8u wanted to use for a situation like this.

Thanks for the feedback. I can improve the documentation on the 8u wiki to make this #clearer. If Skara doesn't recognise the issue as a backport, it doesn't seem to like the referenced bug. Prior to you updating the title, it was issuing a warning that the bug was not open (as we'd expect), so I'm not sure it would correctly resolve it with a backport issue when pushed. Using the "Backport <x>" nomenclature just seems to make things work well in all cases.

I also noticed the previous message about "bug not being open" before the PR was changed to a backport. I assume that is a opportunity-for-improvement for the bots to say "we noticed we'll need to create a new backport record if this gets pushed," which the bots should certainly be able to do.

My general advice on using JBS includes "don't create a backport record before it is needed." I didn't try to create an 8-pool record to see if it would making the warning go away since I assumed the right thing would happen on the push with respect to JBS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

5 participants