8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m #1845

wangweij · 2020-12-18T19:20:47Z

This fix covers both

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issues

JDK-8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m
JDK-8257860: [macOS]: Remove JNF dependency from libosxkrb5/SCDynamicStoreConfig.m

Reviewers

Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to b5f1e15
Phil Race (@prrace - Reviewer) ⚠️ Review applies to ba3d1a1
Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to b5f1e15
Valerie Peng (@valeriepeng - Reviewer) ⚠️ Review applies to b5f1e15

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1845/head:pull/1845
$ git checkout pull/1845

…mpl.m 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m

bridgekeeper · 2020-12-18T19:21:27Z

👋 Welcome back weijun! 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 · 2020-12-18T19:22:34Z

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

core-libs
security

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.

wangweij · 2020-12-18T23:34:08Z

/issue add JDK-8257860

openjdk · 2020-12-18T23:34:34Z

@wangweij
Adding additional issue to issue list: 8257860: [macOS]: Remove JNF dependency from libosxkrb5/SCDynamicStoreConfig.m.

bridgekeeper · 2021-01-15T23:54:33Z

@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

wangweij · 2021-01-21T17:08:05Z

New commit pushed. There is completely no dependency on JavaNativeFramework anymore.

Major change this time: Update the format of SCDynamicStoreConfig::getKerberosConfig.

ToDo: clean up JNIUtilities.h, take back JNFPerformEnvBlock.

reverted:

erikj79 · 2021-01-29T13:42:28Z

/label build

openjdk · 2021-01-29T13:42:35Z

@erikj79 The build label was already applied.

mlbridge · 2021-01-29T14:35:49Z

Webrevs

erikj79

Build changes look good.

openjdk · 2021-01-29T14:46:44Z

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

8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m
8257860: [macOS]: Remove JNF dependency from libosxkrb5/SCDynamicStoreConfig.m

Reviewed-by: erikj, prr, ihse, valeriep

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

b0ee7a8: 8241995: Clarify InetSocketAddress::toString specification
0ef93fe: 8259265: Refactor UncaughtExceptions shell test as java test.
5324b5c: 8260998: Shenandoah: Restore reference processing statistics reporting
c8de943: 8260617: Merge ZipFile encoding check with the initial hash calculation
ae2c5f0: 8260581: IGV: enhance node search
9037615: 8222850: jshell tool: Misleading cascade compiler error in switch expression with undefined vars
91e6c75: 8260928: InitArrayShortSize constraint func should print a helpful error message
cb127a4: 8198343: Test java/awt/print/PrinterJob/TestPgfmtSetMPA.java may fail w/o printer
c008410: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test
b9d4211: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX
... and 44 more: https://git.openjdk.java.net/jdk/compare/f5ca8385abf22678fdc9c965c60d714d20f54100...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.

make/modules/java.security.jgss/Lib.gmk

src/java.base/macosx/classes/apple/security/KeychainStore.java

src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m

only in patch2: unchanged:

wangweij · 2021-01-30T17:13:33Z

Added a test. Unfortunately it has to be manual because updating the dynamic store needs sudo privilege.

AlanBateman · 2021-01-31T11:00:38Z

/label remove core-libs

openjdk · 2021-01-31T11:01:02Z

@AlanBateman
The core-libs label was successfully removed.

magicus

Basically looks good, but the Obj-C test file needs proper handling.

make/test/JtregNativeJdk.gmk

erikj79

Build changes look good. Thanks for fixing the .m support in TestFilesCompilation.gmk!

magicus

Looks good now!

valeriepeng · 2021-02-03T11:27:49Z

src/java.security.jgss/share/classes/sun/security/krb5/SCDynamicStoreConfig.java

+            if (!kdcs.isEmpty()) {
+                ri.put("kdc", kdcs);
+            }
+            realms.put(nextRealm, ri);


Do you need to check if ri is empty?

It should make no difference to applications whether there is an empty section or not, but I can remove it.

valeriepeng

Rest of the Kerberos config handling changes look fine.
Thanks!

wangweij · 2021-02-03T16:19:32Z

/integrate

openjdk · 2021-02-03T16:20:39Z

@wangweij Since your change was applied there have been 55 commits pushed to the master branch:

472bf62: 8258799: [Testbug] RandomCommandsTest must check if tested directive is added via jcmd
b0ee7a8: 8241995: Clarify InetSocketAddress::toString specification
0ef93fe: 8259265: Refactor UncaughtExceptions shell test as java test.
5324b5c: 8260998: Shenandoah: Restore reference processing statistics reporting
c8de943: 8260617: Merge ZipFile encoding check with the initial hash calculation
ae2c5f0: 8260581: IGV: enhance node search
9037615: 8222850: jshell tool: Misleading cascade compiler error in switch expression with undefined vars
91e6c75: 8260928: InitArrayShortSize constraint func should print a helpful error message
cb127a4: 8198343: Test java/awt/print/PrinterJob/TestPgfmtSetMPA.java may fail w/o printer
c008410: 8197825: [Test] Intermittent timeout with javax/swing JColorChooser Test
... and 45 more: https://git.openjdk.java.net/jdk/compare/f5ca8385abf22678fdc9c965c60d714d20f54100...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4a8b5c1.

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

8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreI…

6869b1e

…mpl.m 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Dec 18, 2020

add framework so link succeeds

d5ae9a4

openjdk bot added the build build-dev@openjdk.org label Dec 18, 2020

new SCDynamicStoreConfig return value format

3c20a65

wangweij added 6 commits January 21, 2021 17:00

callback libosxsecurity/KeystoreImpl.m

117cc18

Merge branch 'master' into 8257858

6293730

better macro, no more JNI_COCOA_ENTER

f4a98ca

no more header file

5a3af95

reverted:

do not find class and method in loop

e3d92cb

error check, new JavaStringToNSString

cb02503

mlbridge bot mentioned this pull request Jan 29, 2021

8260616: Removing remaining JNF dependencies in the java.desktop module #2305

Closed

3 tasks

wangweij marked this pull request as ready for review January 29, 2021 14:30

openjdk bot added the rfr Pull request is ready for review label Jan 29, 2021

erikj79 approved these changes Jan 29, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 29, 2021

same behavior as before -- empty realm map

b0ee18a

prrace reviewed Jan 29, 2021

View reviewed changes

phil comment

ba3d1a1

prrace approved these changes Jan 30, 2021

View reviewed changes

wangweij added 2 commits January 30, 2021 10:18

end values should be vectors

a0cad8f

a test

38616b3

only in patch2: unchanged:

Merge branch 'master' into 8257858

f34d3c5

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Jan 31, 2021

move test

ef337f1

magicus suggested changes Feb 1, 2021

View reviewed changes

make/test/JtregNativeJdk.gmk Outdated Show resolved Hide resolved

objc use .m

d418a5f

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 1, 2021

file attr error

b5f1e15

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 1, 2021

erikj79 approved these changes Feb 1, 2021

View reviewed changes

magicus approved these changes Feb 2, 2021

View reviewed changes

valeriepeng reviewed Feb 3, 2021

View reviewed changes

valeriepeng approved these changes Feb 3, 2021

View reviewed changes

valerie comment, remove useless test

41fed31

openjdk bot closed this Feb 3, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 3, 2021

wangweij deleted the 8257858 branch February 3, 2021 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m #1845

8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m #1845

wangweij commented Dec 18, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 18, 2020

openjdk bot commented Dec 18, 2020

wangweij commented Dec 18, 2020

openjdk bot commented Dec 18, 2020

bridgekeeper bot commented Jan 15, 2021

wangweij commented Jan 21, 2021

erikj79 commented Jan 29, 2021

openjdk bot commented Jan 29, 2021

mlbridge bot commented Jan 29, 2021 •

edited

Loading

erikj79 left a comment

openjdk bot commented Jan 29, 2021 •

edited

Loading

wangweij commented Jan 30, 2021

AlanBateman commented Jan 31, 2021

openjdk bot commented Jan 31, 2021

magicus left a comment

erikj79 left a comment

magicus left a comment

valeriepeng Feb 3, 2021

wangweij Feb 3, 2021

valeriepeng left a comment

wangweij commented Feb 3, 2021

openjdk bot commented Feb 3, 2021

8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m #1845

8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m #1845

Conversation

wangweij commented Dec 18, 2020 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Download

bridgekeeper bot commented Dec 18, 2020

openjdk bot commented Dec 18, 2020

wangweij commented Dec 18, 2020

openjdk bot commented Dec 18, 2020

bridgekeeper bot commented Jan 15, 2021

wangweij commented Jan 21, 2021

erikj79 commented Jan 29, 2021

openjdk bot commented Jan 29, 2021

mlbridge bot commented Jan 29, 2021 • edited Loading

Webrevs

erikj79 left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 29, 2021 • edited Loading

wangweij commented Jan 30, 2021

AlanBateman commented Jan 31, 2021

openjdk bot commented Jan 31, 2021

magicus left a comment

Choose a reason for hiding this comment

erikj79 left a comment

Choose a reason for hiding this comment

magicus left a comment

Choose a reason for hiding this comment

valeriepeng Feb 3, 2021

Choose a reason for hiding this comment

wangweij Feb 3, 2021

Choose a reason for hiding this comment

valeriepeng left a comment

Choose a reason for hiding this comment

wangweij commented Feb 3, 2021

openjdk bot commented Feb 3, 2021

wangweij commented Dec 18, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 29, 2021 •

edited

Loading

openjdk bot commented Jan 29, 2021 •

edited

Loading