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

apple builds, supress deprecation warnings #15763

Closed
wants to merge 2 commits into from

Conversation

icing
Copy link
Contributor

@icing icing commented Dec 17, 2024

On apple builds, the gssapi/ldap/securetransport headers deprecate almost everything which leads to a wall of compiler warnings on use in code.

Suppress those warning that may hide other warnings/errors.

On apple builds, the gssapi/ldap/securetransport headers deprecate
almost everything which leads to a wall of compiler warnings on
use in code.

Suppress those warning that may hide other warnings/errors.
@icing icing requested a review from bagder December 17, 2024 08:51
@bagder bagder added the build label Dec 17, 2024
@vszakats vszakats added the appleOS specific to an Apple operating system label Dec 17, 2024
@bagder bagder requested a review from vszakats December 17, 2024 09:11
@testclutch
Copy link

Analysis of PR #15763 at 11cc7ccb:

Test 498 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

Copy link
Member

@vszakats vszakats left a comment

Choose a reason for hiding this comment

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

These warnings are annoying indeed. It's possible to silence them by setting a sufficiently low target version via -mmacosx-version-min= / -DCMAKE_OSX_DEPLOYMENT_TARGET= (details in .github/workflows/macos.yml). But, because Secure Transport requires a high version-min, it's not possible to avoid them with Secure Transport enabled together with GSS/LDAP.
I'm a little bit torn with this. Shouldn't we wait till May 2025, which will allow to solve this by setting a low target version (still to be done manually)? and/or by disabling the deprecated parts by default (or depending on target version, with some complexity)?
On the other hand existing warnings don't seem overly useful either, because these deprecated bits are still supported in latest macOS. Future deprecations might be useful, but curl doesn't use much Apple-specific APIs besides these.

I'm casually in favor, given the above.

If going with it, we might drop most if not all min-version logic and -Wno-deprecated-declarations use from GHA/macos.

@icing
Copy link
Contributor Author

icing commented Dec 17, 2024

I think suppressing it in code via pragmas seems to solve the issues in a very simple way. I do not know of the target selection has other side effects.

@vszakats
Copy link
Member

vszakats commented Dec 17, 2024

Agreed, it's the simplest, and unlikely the warnings will be missed by somebody.

Would you want to give a try to delete min-ver logic and -Wno-deprecated-declarations
from GHA/macos, and see which ones are still necessary after this patch?

edit: Hm, some of those (except 10.8/10.9) may have importance, so perhaps just go with dropping -Wno-deprecated-declarations in this patch and deal with the rest separately?

edit 2: relevant GHA code:

# Deprecated Apple APIs and the macos-version-min value required to avoid
# deprecation warnings with llvm/clang:
#
# - 10.7 Lion (2011) - GSS
# - 10.8 Mountain Lion (2012) - CFURLCreateDataAndPropertiesFromResource (used by curl Secure Transport code)
# - 10.9 Maverick (2013) - LDAP
# - 10.14 Mojave (2018) - Secure Transport
#
# For Secure Transport, curl implements features that require a target
# newer than the 10.8 required by `CFURLCreateDataAndPropertiesFromResource`.
# In this case `-Wno-deprecated-declarations` still comes handy to pacify
# deprecation warnings, though the real solution would be to avoid calling
# that function.

cflags: '-Wno-deprecated-declarations'

CFLAGS+=' ${{ matrix.build.cflags }}'

@github-actions github-actions bot added the CI Continuous Integration label Dec 17, 2024
@icing
Copy link
Contributor Author

icing commented Dec 17, 2024

Agreed, it's the simplest, and unlikely the warnings will be missed by somebody.

Would you want to give a try to delete min-ver logic and -Wno-deprecated-declarations from GHA/macos, and see which ones are still necessary after this patch?

I removed the -Wno-deprecated-declarations from the macos CI job. Lets see.

@icing icing requested a review from vszakats December 17, 2024 13:08
@bagder bagder closed this in ef90ee3 Dec 17, 2024
vszakats added a commit that referenced this pull request Dec 17, 2024
Drop them, except for Secure Transport jobs where they may trigger
different code paths.

Also drop unused `matrix.build.cflags` variable.

Follow-up to ef90ee3 #15763
Closes #15766
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 build CI Continuous Integration
Development

Successfully merging this pull request may close these issues.

4 participants