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

8296812: sprintf is deprecated in Xcode 14 #1930

Closed
wants to merge 4 commits into from

Conversation

elifaslan1
Copy link
Contributor

@elifaslan1 elifaslan1 commented Oct 27, 2023

https://developer.apple.com/news/?id=y5mjxqmn

Starting November 1, 2023, the Apple notary service will no longer accept uploads from altool or Xcode 13 or earlier.

This is not a clean backport and this pull request contains a backport of the closed bug JDK-8296812 commit f1522c648c2e.

Conflicts includes:

  • whitespace conflicts on src/hotspot/share/utilities/utf8.cpp

  • missing src/hotspot/share/jfr/support/jfrSymbolTable.cpp

  • context difference on src/hotspot/cpu/aarch64/vm_version_aarch64.cpp

The sprintf is deprecated in Xcode 14 and the use of it causing building failure. The build could pass if warnings are disabled for codes that use sprintf method. For the long run, the sprintf could be replaced with snprintf. This patch is trying to check if snprintf could be used.

Testing:

GHA tested.
Successfull tier1,tier2,tier3 and jck tests for linux platforms.
Successfull tier1 and tier2 for windows.


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
  • JDK-8296812 needs maintainer approval

Issue

  • JDK-8296812: sprintf is deprecated in Xcode 14 (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1930

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 27, 2023

👋 Welcome back elifaslan1! 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 changed the title Backport 478ef389dc3767edfbe21d10a7f7f1522c648c2e 8296812: sprintf is deprecated in Xcode 14 Oct 27, 2023
@openjdk
Copy link

openjdk bot commented Oct 27, 2023

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

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Oct 27, 2023
@mlbridge
Copy link

mlbridge bot commented Oct 27, 2023

Webrevs

@elifaslan1
Copy link
Contributor Author

/approval request

@openjdk
Copy link

openjdk bot commented Oct 30, 2023

@elifaslan1
8296812: The approval request has been created successfully.

@openjdk openjdk bot added the approval label Oct 30, 2023
@GoeLin
Copy link
Member

GoeLin commented Oct 31, 2023

Hi @elifaslan1, please first get a review before you label for approval. Also, be a bit more elaborate in the fix request message, see also the instructions in . All test after "request" will be copied into the JBS comment.https://wiki.openjdk.org/display/JDKUpdates/How+to+contribute+or+backport+a+fix

@openjdk openjdk bot removed the approval label Oct 31, 2023
@elifaslan1 elifaslan1 closed this Nov 1, 2023
@elifaslan1 elifaslan1 deleted the JDK-8296812 branch November 1, 2023 21:58
@elifaslan1 elifaslan1 restored the JDK-8296812 branch November 2, 2023 00:47
@elifaslan1 elifaslan1 reopened this Nov 2, 2023
@openjdk openjdk bot added approval and removed approval labels Nov 9, 2023
@bridgekeeper
Copy link

bridgekeeper bot commented Nov 28, 2023

@elifaslan1 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!

@elifaslan1
Copy link
Contributor Author

@shipilev could you please review this PR. TIA

@shipilev
Copy link
Member

Sure, I will try to take a look at this.

@ysramakrishna
Copy link
Member

I don't see the changes for the gtest covered in https://github.com/openjdk/jdk/pull/11793/files

There might be others. You could confirm by running a find and grep for sprintf( over the repo to ensure you got them all.

Note that https://bugs.openjdk.org/browse/JDK-8299635 has a couple of linked tickets in which further changes were made by @XueleiFan. Would be good for @XueleiFan to also do a review.

You could also do the build on osx, then run a symbol check over all of the .so's in the build image to ensure that there are no remaining references to the deprecated method.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 29, 2023

@elifaslan1 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!

Copy link
Member

@ysramakrishna ysramakrishna left a comment

Choose a reason for hiding this comment

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

See previous comment in conversation.

@elifaslan1 elifaslan1 closed this Jan 6, 2024
@elifaslan1 elifaslan1 deleted the JDK-8296812 branch January 6, 2024 21:53
@elifaslan1 elifaslan1 restored the JDK-8296812 branch January 6, 2024 21:59
@elifaslan1 elifaslan1 reopened this Jan 6, 2024
Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

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

This looks generally fine, but I think there is an accidental change that crept in. Otherwise the backport looks similar to what mainline does.

I see there are many linked issues to this bug, but all of them look like dealing with other places, and they should probably be considered for future backports.

#define ADD_FEATURE_IF_SUPPORTED(id, name, bit) if (_features & CPU_##id) strcat(buf, ", " name);
int buf_used_len = os::snprintf_checked(buf, sizeof(buf), "0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, _revision);
if (_model2) os::snprintf_checked(buf + buf_used_len, sizeof(buf) - buf_used_len, "(0x%03x)", _model2);
#define ADD_FEATURE_IF_SUPPORTED(id, name, bit) if (_features & CPU_##id) strcat(buf, ", " #name);
Copy link
Member

Choose a reason for hiding this comment

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

Original commit does not add # to #name.

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 15, 2024

@elifaslan1 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Feb 15, 2024
@zzambers
Copy link
Contributor

zzambers commented Jul 1, 2024

It would be nice to have this one finished, as macos-11 seems to have been dropped in GHA. Sprintf fixes are required by JDK-8318039 (GHA: Bump macOS and Xcode versions).

@jerboaa
Copy link
Contributor

jerboaa commented Jul 4, 2024

There is a new PR: #2662

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

Successfully merging this pull request may close these issues.

6 participants