Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Mac Catalina build, suppress warnings #39938

Merged
buyaa-n merged 3 commits intodotnet:masterfrom
buyaa-n:mac_catalina_build
Aug 1, 2019
Merged

Mac Catalina build, suppress warnings #39938
buyaa-n merged 3 commits intodotnet:masterfrom
buyaa-n:mac_catalina_build

Conversation

@buyaa-n
Copy link

@buyaa-n buyaa-n commented Aug 1, 2019

No description provided.

@buyaa-n buyaa-n requested review from bartonjs and danmoseley August 1, 2019 00:50
@buyaa-n buyaa-n added the os-mac-os-x OS-X aka Mac OS label Aug 1, 2019
@danmoseley danmoseley requested a review from krwq August 1, 2019 03:11
@wfurt
Copy link
Member

wfurt commented Aug 1, 2019

It builds for me on 10.15 without any changes. What Xcode version do you use? It may be better to adjust cmake rules than fix every occurrence.

@buyaa-n
Copy link
Author

buyaa-n commented Aug 1, 2019

What Xcode version do you use?

it was XCode 11, downloaded latest version

It may be better to adjust cmake rules than fix every occurrence.

Yeah, as it was not that many did it for every occurance as we might want to be notified with deprecated functionalies for other areas too

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

It turned out I was on 10.3. When using 11 Beta I saw same warnings/errors and this change fix them.

@krwq
Copy link
Member

krwq commented Aug 1, 2019

Thanks @buyaa-n

@buyaa-n buyaa-n merged commit c4e7347 into dotnet:master Aug 1, 2019
buyaa-n added a commit to buyaa-n/corefx that referenced this pull request Aug 2, 2019
@karelz karelz added this to the 5.0 milestone Aug 3, 2019
@stephentoub
Copy link
Member

The fact that these warnings are firing suggests that there's something better we should be using instead of these existing calls?

@bartonjs
Copy link
Member

bartonjs commented Aug 5, 2019

The MD5 ones, at least, are "MD5 is broken, you shouldn't be using it". Except for all the places it's needed, of course. Otherwise they've added some stuff for 10.15, and deprecated the alternatives. But that doesn't help us on 10.13 or 10.14; so it's not useful for at least another two years (or we split our distribution from one build to one build per OS version).

@stephentoub
Copy link
Member

or we split our distribution from one build to one build per OS version

And that's what I'm wondering. It seemed like there were some other issues still being investigated that might cause us to need to do that? If that's the case, disabling these warnings seems premature. If it turns out that's a red herring, great, let's proceed to port this disabling to 3.0. But if that's actually required, seems we should hold off on #39984 and port the "right" fix?

@bartonjs
Copy link
Member

bartonjs commented Aug 5, 2019

For 3.0 I don't think we'd rewrite SslStream on top of their new TLS stack in servicing; nor replace X509Chain with an easier to use, but less functionally capable (it only reports one code per chain), implementation of cert chaining. If we already have to split the distribution then maybe for 3.1 or 5.0 we'd change things... but I think it'd have to be for something actually "better" vs just "different"... especially when we have to keep the old code around anyways.

@buyaa-n
Copy link
Author

buyaa-n commented Aug 5, 2019

Issue #40031 created for tracking further upgrade

buyaa-n added a commit to buyaa-n/corefx that referenced this pull request Aug 6, 2019
wtgodbe pushed a commit that referenced this pull request Aug 7, 2019
#39984)

* For Mac Catalina build, suppress warnings  (#39938)

* Applying feedback
@buyaa-n buyaa-n deleted the mac_catalina_build branch November 1, 2019 20:55
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

os-mac-os-x OS-X aka Mac OS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants