8295554: Move the "sizecalc.h" to the correct location #10757

mrserb · 2022-10-19T08:15:54Z

The "sizecalc.h" file is moved out from java.desktop/share/native/include, the files in that folder appear in the jdk bundle after installation like the "jawt.h".

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-8295554: Move the "sizecalc.h" to the correct location

Reviewers

Erik Joelsson (@erikj79 - Reviewer)
Magnus Ihse Bursie (@magicus - Reviewer)
Phil Race (@prrace - Reviewer)
Alexander Zuev (@azuev-java - Reviewer)
Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10757

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10757.diff

bridgekeeper · 2022-10-19T08:16:47Z

👋 Welcome back serb! 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 · 2022-10-19T08:18:55Z

@mrserb The following labels will be automatically applied to this pull request:

build
client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mrserb · 2022-10-20T04:15:33Z

make/common/modules/LauncherCommon.gmk

@@ -45,7 +45,6 @@ endif

 LAUNCHER_SRC := $(TOPDIR)/src/java.base/share/native/launcher
 LAUNCHER_CFLAGS += -I$(TOPDIR)/src/java.base/share/native/launcher \
-    -I$(TOPDIR)/src/java.desktop/share/native/include \


This line was added recently, not sure why.

Hmm .. IIRC Alex Zuev said there is a (closed) native test and adding this was the only way he could make that test work ..
Now clearly this location no longer makes sense when the file is moved, but someone will need to do something about the test. It needs Alex to chime in here.

Superficially, it looks like the closed test has a direct pointer to where to get this which would need to be re-adjusted
but now I really do need Alex to explain why the above include dir was needed .. since it isn't for the closed test.

Superficially, it looks like the closed test has a direct pointer to where to get this which would need to be re-adjusted

The test has direct link to the include yet i had to add this line so jtreg could find it in the build phase. The fact that it will lead to publishing of this include was not an intentional consequence and it needs to be changed, probably the test should be adjusted later if it will start failing on certain platforms.

mlbridge · 2022-10-20T04:53:47Z

Webrevs

00: Full (567cfb2b)

openjdk · 2022-10-20T18:03:06Z

@mrserb 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:

8295554: Move the "sizecalc.h" to the correct location

Reviewed-by: erikj, ihse, prr, kizune, aivanov

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

7520d0a: 8295855: ProblemList jdk/jshell/CommandCompletionTest.java on linux-all
1d15e5c: 8295716: Minimize disabled warnings in security libs
8c86e92: 8295847: slow debug build error after JDK-8294466
e122321: 8295844: jdk/test/whitebox/CPUInfoTest.java failed with "not all features are known: expected true, was false"
df81b3c: 8295738: Automate javax/swing/JFileChooser/FileSizeCheck.java
68cf248: 8295298: Automate javax/swing/JFileChooser/FileViewNPETest.java
fefbddf: 8291443: Obsolete the PrintSharedDictionary flag
5b3de6e: 8284840: Update CLDR to Version 42.0
2f3f3b6: 8295792: Clean up old async close code
5ac6f18: 8290010: IGV: Fix UndoRedo Action
... and 47 more: https://git.openjdk.org/jdk/compare/8d4c077218748d37617fc1bdb537a165706a5849...master

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.

magicus

Can you please refrain from integrating this while I investigate while this was originally moved?

mrserb · 2022-10-21T19:06:13Z

Can you please refrain from integrating this while I investigate while this was originally moved?

sure.

prrace · 2022-10-23T00:37:55Z

Can you please refrain from integrating this while I investigate while this was originally moved?

sure.

It was moved because it was designed for use by AWT and SOMEHOW during all the reshuffle for the modular JDK in JDK 9 it ended up in base, which doesn't use it and doesn't want it - it belongs in the desktop module.

TheShermanTanker · 2022-10-24T09:55:58Z

Can you please refrain from integrating this while I investigate while this was originally moved?

sure.

It was moved because it was designed for use by AWT and SOMEHOW during all the reshuffle for the modular JDK in JDK 9 it ended up in base, which doesn't use it and doesn't want it - it belongs in the desktop module.

I guess that explains why I was so confused when I found this in java.base and thought it was a codebase wide utility, except that I couldn't find it being used anywhere else other than java.desktop :P

mrserb · 2022-10-24T20:10:02Z

I suggest to disable that test, if it builds during the jdk build as part of test bundle it may cause the whole jdk build to fail in mach5.

prrace · 2022-10-24T20:40:05Z

@aivanov-jdk has prepared a patch which will be pushed to fix this up but I agree the timing needs to be perfect to be safe here .. if you can't co-ordinate a time with him then he would need to disable it now and re-enable it later.

magicus

Thank you for waiting while we sorted out the details. This looks good to push. However, as prrace says, we have a corresponding closed part that needs to go in at the same time. One way to solve this is that you give the /integrate defer PR command. Then any jdk committer can actually integrate the PR later, and make sure it is coordinated with our closed counterpart.

mrserb · 2022-10-24T21:58:21Z

/integrate defer

openjdk · 2022-10-24T22:01:15Z

@mrserb Integration of this pull request has been deferred and may be completed by any project committer using the /integrate pull request command.

aivanov-jdk · 2022-10-25T09:43:57Z

/integrate

openjdk · 2022-10-25T09:45:45Z

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

706d1b7: 8295798: (ch) Test java/nio/channels/Channels/ReadXBytes.java is very slow on Windows
89dafc0: 8292699: Improve printing of classes in native debugger
7520d0a: 8295855: ProblemList jdk/jshell/CommandCompletionTest.java on linux-all
1d15e5c: 8295716: Minimize disabled warnings in security libs
8c86e92: 8295847: slow debug build error after JDK-8294466
e122321: 8295844: jdk/test/whitebox/CPUInfoTest.java failed with "not all features are known: expected true, was false"
df81b3c: 8295738: Automate javax/swing/JFileChooser/FileSizeCheck.java
68cf248: 8295298: Automate javax/swing/JFileChooser/FileViewNPETest.java
fefbddf: 8291443: Obsolete the PrintSharedDictionary flag
5b3de6e: 8284840: Update CLDR to Version 42.0
... and 49 more: https://git.openjdk.org/jdk/compare/8d4c077218748d37617fc1bdb537a165706a5849...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-25T09:46:07Z

@aivanov-jdk Pushed as commit 6673cd8.

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

aivanov-jdk · 2022-10-25T15:38:30Z

Thanks, @magicus, for the suggestion of using /integrate defer, it worked like a charm. Both changes went into the same build.

Thanks, @mrserb, for catching and resolving the issue and for cooperating.

8295554: Move the "sizecalc.h" to the correct location

1836ee6

openjdk bot added build build-dev@openjdk.org client client-libs-dev@openjdk.org labels Oct 19, 2022

Merge remote-tracking branch 'upstream/master' into JDK-8295554

567cfb2

mrserb commented Oct 20, 2022

View reviewed changes

mrserb marked this pull request as ready for review October 20, 2022 04:47

openjdk bot added the rfr Pull request is ready for review label Oct 20, 2022

erikj79 approved these changes Oct 20, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 20, 2022

magicus suggested changes Oct 21, 2022

View reviewed changes

prrace approved these changes Oct 24, 2022

View reviewed changes

azuev-java approved these changes Oct 24, 2022

View reviewed changes

aivanov-jdk approved these changes Oct 24, 2022

View reviewed changes

magicus approved these changes Oct 24, 2022

View reviewed changes

openjdk bot added the deferred label Oct 24, 2022

openjdk bot added the integrated Pull request has been integrated label Oct 25, 2022

openjdk bot closed this Oct 25, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review deferred labels Oct 25, 2022

mrserb deleted the JDK-8295554 branch October 25, 2022 16:29

mrserb mentioned this pull request Nov 1, 2022

8294488: Delete KCMS transforms wrappers #10459

Closed

3 tasks

8295554: Move the "sizecalc.h" to the correct location #10757

8295554: Move the "sizecalc.h" to the correct location #10757

mrserb commented Oct 19, 2022 •

edited by openjdk bot

bridgekeeper bot commented Oct 19, 2022

openjdk bot commented Oct 19, 2022

mrserb Oct 20, 2022

prrace Oct 23, 2022

prrace Oct 23, 2022 •

edited

azuev-java Oct 24, 2022

mlbridge bot commented Oct 20, 2022

openjdk bot commented Oct 20, 2022 •

edited

magicus left a comment

mrserb commented Oct 21, 2022

prrace commented Oct 23, 2022

TheShermanTanker commented Oct 24, 2022

mrserb commented Oct 24, 2022

prrace commented Oct 24, 2022

magicus left a comment

mrserb commented Oct 24, 2022

openjdk bot commented Oct 24, 2022

aivanov-jdk commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

aivanov-jdk commented Oct 25, 2022

8295554: Move the "sizecalc.h" to the correct location #10757

8295554: Move the "sizecalc.h" to the correct location #10757

Conversation

mrserb commented Oct 19, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 19, 2022

openjdk bot commented Oct 19, 2022

mrserb Oct 20, 2022

Choose a reason for hiding this comment

prrace Oct 23, 2022

Choose a reason for hiding this comment

prrace Oct 23, 2022 • edited

Choose a reason for hiding this comment

azuev-java Oct 24, 2022

Choose a reason for hiding this comment

mlbridge bot commented Oct 20, 2022

Webrevs

openjdk bot commented Oct 20, 2022 • edited

magicus left a comment

Choose a reason for hiding this comment

mrserb commented Oct 21, 2022

prrace commented Oct 23, 2022

TheShermanTanker commented Oct 24, 2022

mrserb commented Oct 24, 2022

prrace commented Oct 24, 2022

magicus left a comment

Choose a reason for hiding this comment

mrserb commented Oct 24, 2022

openjdk bot commented Oct 24, 2022

aivanov-jdk commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

openjdk bot commented Oct 25, 2022

aivanov-jdk commented Oct 25, 2022

mrserb commented Oct 19, 2022 •

edited by openjdk bot

prrace Oct 23, 2022 •

edited

openjdk bot commented Oct 20, 2022 •

edited