Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Cherry picking Mojave fix (#30716) to release 2.1 #30744

Merged
merged 1 commit into from
Jul 5, 2018
Merged

Cherry picking Mojave fix (#30716) to release 2.1 #30744

merged 1 commit into from
Jul 5, 2018

Conversation

maryamariyan
Copy link
Member

Related to issue #30599
Cherry picks PR #30716

cc: @danmosemsft @bartonjs

@maryamariyan
Fix compilation for deprecated API on macOS Mojave preview (dotnet#30716
27025a2 
)

* Fix compilation for deprecated API on macOS Mojave preview

Fixes: #30599

* Fixing tests on macOS Mojave
@maryamariyan maryamariyan added os-mac-os-x OS-X aka Mac OS * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) labels Jun 28, 2018
@maryamariyan maryamariyan self-assigned this Jun 28, 2018
@maryamariyan maryamariyan changed the base branch from master to release/2.1 June 28, 2018 23:30
bartonjs
bartonjs reviewed Jun 29, 2018
View reviewed changes
src/Native/Unix/System.Security.Cryptography.Native.Apple/CMakeLists.txt
@@ -26,6 +26,9 @@ set(NATIVECRYPTO_SOURCES
pal_x509chain.cpp
)

# Temporary workaround for dotnet/corefx issue #30599
add_compile_options(-Wno-deprecated-declarations)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't need this on the public beta for Mojave (not sure why, but I didn't).

I'm also not sure if it's important that things build on Mojave (this file), just execute (the other file). Particularly if we anticipate a different fix (given the "Temporary" in the comment here)

Copy link
Member

Choose a reason for hiding this comment

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

It's weird you didn't need it. Did you also upgrade your Xcode? It happened to me with both Xcode 10 betas that were released so far.

Copy link
Member

Choose a reason for hiding this comment

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

Either 10 beta or 10 beta2, yeah. Because I tried installing the 10 beta2 CLI tools, things (Homebrew) still detected 9.x, then I "installed" the 10 beta (1) UI, did more things, and eventually figured out that by "installed" I had "downloaded and extracted" it. Then finally figured out how to make that work.

When I was confused I dug out the header file, it did have the deprecated attribute (indicating 10.14 as the upper range), and I stayed confused.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the Xcode installation can be a bit of a pain if you want to keep different versions around...

xcode-select -p
/Applications/Xcode-beta.app/Contents/Developer

@danmoseley
Copy link
Member

danmoseley commented Jul 3, 2018 •
edited
Loading

@bartonjs for 2.1 servicing, do you want to take just this change, or do we have a reason to bring over #30815 as well?

It seems that this change is sufficient to keep building and running on Mojave working, and it's less churn/risk than using the new API in servicing (that's real fix for master)

@bartonjs
Copy link
Member

bartonjs commented Jul 5, 2018

I'm fine with whichever fix for servicing. I wouldn't expect Apple to be so aggressive as to the removal of the old function to cause us a problem during the support lifetime; and if we ever hit a problem we can take the other one.

@danmoseley
Copy link
Member

Ok let's take just this smaller fix to ship room. @maryamariyan Could you please add a row to the table again? I deleted the previous one while we waited...

@danmoseley
Copy link
Member

@dotnet-bot test OSX x64 Debug Build please

Approved by shiproom - merge when green

@maryamariyan maryamariyan merged commit 87b79bd into dotnet:release/2.1 Jul 5, 2018
@maryamariyan maryamariyan deleted the cp-mojave branch July 5, 2018 19:00
@karelz karelz added this to the 2.1.x milestone Jul 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bartonjs bartonjs bartonjs left review comments

@filipnavara filipnavara filipnavara left review comments

Assignees

@maryamariyan maryamariyan

Labels
area-Infrastructure-libraries * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) os-mac-os-x OS-X aka Mac OS
Projects
None yet
Milestone
2.1.x
Development

Successfully merging this pull request may close these issues.
5 participants
@maryamariyan @danmoseley @bartonjs @filipnavara @karelz