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

macOS: use SDK's libCurl. #1542

Merged
merged 3 commits into from Oct 24, 2021
Merged

Conversation

MaddTheSane
Copy link
Contributor

Use the SDK's provided libcurl instead of relying on an outdated stub library.

@mikedld
Copy link
Member

mikedld commented Nov 28, 2020

We currently support macOS 10.10 and up while require latest(-ish) Xcode to build. Using libcurl stud library and headers from SDK installed with Xcode 11/12 could lead to unintended use of features added in later libcurl versions than the one bundled with macOS 10.10 which is obviously bad. Using older SDK means either using older Xcode (due to how SDKs are provided by Apple) or extracting that older SDK from older Xcode and using it with newer Xcode (which may work, but we don't really need the full SDK).

IMHO existing approach is good enough.

@MaddTheSane
Copy link
Contributor Author

One problem: the stub libraries have no ARM64 section, so compiling for Apple Silicon fails.

@mikedld
Copy link
Member

mikedld commented Nov 28, 2020

The fact that arm64 is only supported from macOS 11 doesn't mean that Transmission has to drop support for earlier macOS versions in its x86_64 builds (at least not yet, I would imagine).

@MaddTheSane
Copy link
Contributor Author

Agreed.

I know that Apple modifies the zlib headers to mark availability (when they were "added" to macOS), emitting a warning and weak importing of functions when targeting an older version of macOS. Maybe Apple does the same for libCurl?

See also #1528: libcurl isn't the only stub library that Transmission uses.

@livings124
Copy link
Member

Can we test if a warning is emitted? Was there any new functionality added since the 10.10 version that we can quickly just check with a test compile?

If there is a warning, I'd prefer to just use the OS's version (and even if not, I'm not fully opposed).

@livings124
Copy link
Member

Can we test if a warning is emitted? Was there any new functionality added since the 10.10 version that we can quickly just check with a test compile?

If there is a warning, I'd prefer to just use the OS's version (and even if not, I'm not fully opposed).

@MaddTheSane ping on this

@MaddTheSane
Copy link
Contributor Author

No warning is emitted.

@livings124
Copy link
Member

Oh that's not ideal.

Even so, I don't see it as a hard no to migrating to the SDK's libcurl. We would just need to be more aggressive when new OSs are released.

@ckerr
Copy link
Member

ckerr commented Oct 18, 2021

Oh that's not ideal.

Even so, I don't see it as a hard no to migrating to the SDK's libcurl. We would just need to be more aggressive when new OSs are released.

@livings124 so what do you want to do with this PR? 🙂

@ckerr ckerr added scope:mac dependencies Pull requests that update a dependency file needs clarification More info is needed before work can proceed labels Oct 18, 2021
@livings124
Copy link
Member

livings124 commented Oct 24, 2021

Oh that's not ideal.

Even so, I don't see it as a hard no to migrating to the SDK's libcurl. We would just need to be more aggressive when new OSs are released.

@livings124 so what do you want to do with this PR? 🙂

I'm alright with merging this. I think we'd catch any api changes soon after an os beta is released.

@mikedld
Copy link
Member

mikedld commented Oct 24, 2021

I'm going to look into possibly setting up an agent to test on target/minimal macOS version, which would hopefully be enough once we have some more unit tests written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file needs clarification More info is needed before work can proceed scope:mac
Development

Successfully merging this pull request may close these issues.

None yet

4 participants