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

explicitly include MacTypes.h #13429

Closed
wants to merge 3 commits into from

Conversation

isaactorz
Copy link

The latest macos sdk release - 15.3, seems to have broken a transitive include for OSStatus types used in sectransp.c

The fix here is to explicitly include the header that declares these symbols.

The latest macos sdk release - 15.3, seems to have broken a transitive
include for OSStatus types used in sectransp.c

The fix here is to explicitly include the header that declares these
symbols.
@github-actions github-actions bot added TLS appleOS specific to an Apple operating system labels Apr 19, 2024
@isaactorz
Copy link
Author

It turns out this is not the only missing include...

Will update in a sec

lib/vtls/sectransp.c Outdated Show resolved Hide resolved
This reverts commit 9876b6e.
@vszakats vszakats changed the title explicitly include MacosTypes explicitly include MacTypes.h Apr 24, 2024
@isaactorz
Copy link
Author

It turns out this is not the only missing include...

Will update in a sec

This turned out to be an issue with our build configurations - but the MacTypes issue was valid.

@nickzman
Copy link
Member

Is it? I'm able to build the latest source using Xcode 15.3 with no problems, so I'm not convinced this change is needed.

@bagder
Copy link
Member

bagder commented May 2, 2024

@isaactorz ?

@isaactorz
Copy link
Author

Is it? I'm able to build the latest source using Xcode 15.3 with no problems, so I'm not convinced this change is needed.

Sorry for the delayed response.

I think I might've jumped the gun with this change - I meant valid in the sense that it's a transitive include that AFAIK isn't pulled in by some dedicated omnibus header.

We got caught off guard by lots of mac builds suddenly breaking around this and related errors and this seemed to be necessary to fix the problem - so I wanted to upstream it quickly in case we could avoid using a fork or adding custom patches.

What I ended up doing is re-evaluating why we're building macos native (and deprecated TLS), when we are already using openssl. So I ended up just building curl against that to fix the problem. We are also using a non-standard build system so this error is going to be hard to reproduce outside of our codebase.

In the end this is an extremely trivial change that'll unlikely to majorly hurt or help anyone so I am totally ok with closing this!

Thanks for the responsiveness and I'll be careful to not jump the gun if I ever end up contributing here again in the future.

@jay
Copy link
Member

jay commented May 9, 2024

Thanks for the update, closing

@jay jay closed this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appleOS specific to an Apple operating system TLS
Development

Successfully merging this pull request may close these issues.

None yet

4 participants