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
cmake: delete obsolete TODOs more [ci skip] #12509
Conversation
- 898b012 curl#1288 - 5de6848 curl#10023 - bunch of others that are completed - `NTLM_WB_ENABLED` is implemented in a basic form, and now also scheduled for removal, so a TODO at this point isn't useful. And this 'to-check' item: Q: "The cmake build selected to run gcc with -fPIC on my box while the plain configure script did not." A: With CMake, since 2ebc74c curl#11546 and fc9bfb1 curl#11627, we explicitly enable PIC for libcurl shared lib. Or when building libcurl for shared and static lib in a single pass. We do this by default for Windows or when enabled by the user via `SHARE_LIB_OBJECT`. Otherwise we don't touch this setting. Meaning the default set by CMake (if any) or the toolchain is used. On Debian Bookworm, this means that PIC is disabled for static libs by default. Some platforms (like macOS), has PIC enabled by default. autotools supports the double-pass mode only, and in that case CMake seems to match PIC behaviour now (as tested on Linux with gcc.) Follow-up to 5d5dfdb curl#12500 Closes #xxxxx
Ideally this should be noted somewhere other than the commit message |
I think the point is that it now works as it should (if my tests were accurate), and therefore need no special documentation. If it'd help, I'm happy to include it in this commit, but not volunteering to write it myself. |
Alright well do you think something like this is worth it: diff --git a/docs/INSTALL.cmake b/docs/INSTALL.cmake
index 4e7f706..b103da5 100644
--- a/docs/INSTALL.cmake
+++ b/docs/INSTALL.cmake
@@ -87,3 +87,11 @@ cmake-gui
GUI. Once you have selected all the options you want, click the
"Generate" button.
6. Run the native build tool that you used CMake to generate.
+
+Notes
+=====
+ If you build curl as a shared library then the position indepenent code
+ option is enabled if the compiler supports it. If you build curl as a shared
+ library and a static library at the same time,
+ like `-DBUILD_SHARED_LIBS=ON -DBUILD_STATIC_LIBS=ON`, then the position
+ independent code option is enabled for both if the compiler supports it. |
With |
Nevermind. Though adding PIC may be unexpected, researching it just now I don't think it's a cause for concern. |
PIC is an almost cost-free on 64-bit systems 1. It's also a prerequisite for ASLR. Hence on Windows I think it is safe to have enabled by default (it's also the compiler default with Windows compiler). On macOS/iOS store apps it has been a requirement since 2011. It's also the default in Xcode, so even there it'd be safe to enable by default. On *nix systems the picture is less clear but most (non-embedded) systems are likely shifting into this direction or already there, I don't know. In curl-for-win Linux builds I enabled it. [ I'm in favour of PIC, unless there is a specific reason not to use it. ] Footnotes |
NTLM_WB_ENABLED
is implemented in a basic form, and now alsoscheduled for removal, so a TODO at this point isn't useful.
And this 'to-check' item:
Q: "The cmake build selected to run gcc with -fPIC on my box while the
plain configure script did not."
A: With CMake, since 2ebc74c #11546
and fc9bfb1 #11627, we explicitly
enable PIC for libcurl shared lib. Or when building libcurl for
shared and static lib in a single pass. We do this by default for
Windows or when enabled by the user via
SHARE_LIB_OBJECT
.Otherwise we don't touch this setting. Meaning the default set by
CMake (if any) or the toolchain is used. On Debian Bookworm, this
means that PIC is disabled for static libs by default. Some platforms
(like macOS), has PIC enabled by default.
autotools supports the double-pass mode only, and in that case
CMake seems to match PIC behaviour now (as tested on Linux with gcc.)
Follow-up to 5d5dfdb #12500
Closes #12509