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

cmake: option to disable install & drop curlu target when unused #12287

Closed
wants to merge 7 commits into from
Closed

cmake: option to disable install & drop curlu target when unused #12287

wants to merge 7 commits into from

Conversation

i1qh2n
Copy link
Contributor

@i1qh2n i1qh2n commented Nov 6, 2023

This patch makes the following changes:

  • adds the option CURL_DISABLE_INSTALL - to disable 'install' targets.
  • Removes the target curlu when the option BUILD_TESTING is set to OFF - to prevent it from being loaded in Visual Studio.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
lib/CMakeLists.txt Outdated Show resolved Hide resolved
lib/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@vszakats
Copy link
Member

vszakats commented Nov 6, 2023

Can you add a few words about why the new if(BUILD_TESTING) conditions are necessary to disable install?

@i1qh2n
Copy link
Contributor Author

i1qh2n commented Nov 6, 2023

It is my understanding that the curlu library is only needed in when building tests. With that it falls under the same category as installation targets. In that it is not needed when building inline unless the option BUILD_TESTING is set to ON

@vszakats
Copy link
Member

vszakats commented Nov 6, 2023

Did those targets get built for you with BUILD_TESTING=OFF? If so, can you post the cmake settings to trigger it?

@i1qh2n
Copy link
Contributor Author

i1qh2n commented Nov 6, 2023

With the default configuration on this repository:
Setting BUILD_TESTING to OFF produces the projects

ALL_BUILD
curl 
curl_uninstall
curltool
INSTALL
libcurl_object
libcurl_shared
ZERO_CHECK

Setting BUILD_TESTING to ON produces the projects

ALL_BUILD
curl 
curl_uninstall
curltool
curlu
INSTALL
libcurl_object
libcurl_shared
Test lib ...
Test lib ...
ZERO_CHECK

I have this repository embedded in an experimental project that uses curl as a submodule.

The initial example is what I am using to lock the options I need in place before recursing into this repository’s scripts.

Toggling BUILD_TESTING=ON|OFF from that level achieves the same result as if I were toggling it on the command line.

For instance, if I wanted to include this project’s tests as part of my project testing, I would conditionally switch that variable to ON at the scope before add_subdirectory(libcurl). That in turn would toggle on the curlu project as well as all the Test lib … projects to include as part of my build.

I added the conditional BUILD_TESTING statement around curlu because it was not needed (outside of testing) to link and use the main output libraries.

CMakeLists.txt Outdated Show resolved Hide resolved
@vszakats
Copy link
Member

vszakats commented Nov 7, 2023

Your results say that BUILD_TESTING is working as expected, meaning curlu (static lib) is not built when BUILD_TESTING is set to OFF. In that case, do you think we still need the extra guards added in this patch?

@i1qh2n
Copy link
Contributor Author

i1qh2n commented Nov 7, 2023

That is a miscommunication on my end, the above settings are only with the extra guards in place.
From a fresh clone, setting BUILD_TESTING to OFF produces the projects:

ALL_BUILD
curl 
curl_uninstall
curltool
curlu
INSTALL
libcurl_object
libcurl_shared
ZERO_CHECK

With curlu being present in both.

@vszakats
Copy link
Member

vszakats commented Nov 7, 2023

What do you mean by 'project' in this context?

The reason I'm insisting is because this test:

cmake . -B bld -DBUILD_TESTING=OFF

when built, does not produce a curlu library. I'm assuming the reason for this is that even though curlu is defined as a target, it never gets used because BUILD_TESTING=ON is required to build things dependent on it under tests. Without building tests, curlu as a target is not referenced, thus also not built.

I'm looking for an example which builds curlu with BUILD_TESTING=OFF.

@i1qh2n
Copy link
Contributor Author

i1qh2n commented Nov 7, 2023

I see what you are saying. By ‘project’ I am referring to visual studio project files.

Even if it is not referenced in the actual build, the project file is still generated. In turn, it is loaded in the visual studio solution.

By default, the ALL_BUILD project will build both it and either libcurl_static or libcurl_shared depending on settings.

The purpose of this patch is to add the option to toggle off the installation projects in the generated solution. With the BUILD_TESTING condition , it removes the unreferenced project when disabled, or adds it back when enabled.

It allows a strict filter to only generate and include the core libraries needed to build, link and use curl.

@vszakats
Copy link
Member

vszakats commented Nov 7, 2023

Thanks for explaining!

If I got it correctly this patch does these two things:

  1. add option CURL_DISABLE_INSTALL to disable 'install'.
  2. disable test targets more when BUILD_TESTING is OFF, to avoid them appearing in Visual Studio.

Would you mind updating the PR message to clarify?

@i1qh2n
Copy link
Contributor Author

i1qh2n commented Nov 7, 2023

Would you mind updating the PR message to clarify?

Done.

Thank you for your time!

@vszakats
Copy link
Member

vszakats commented Nov 7, 2023

Thank you, LGTM!

@vszakats vszakats changed the title Option to disable CMake install cmake: option to disable install & drop curlu target when unused Nov 7, 2023
@vszakats vszakats closed this in aace27b Nov 10, 2023
vszakats added a commit that referenced this pull request May 16, 2024
Before this patch `BUILD_TESTING` was used once, then initialized, then
used again. This caused the `curlu` library not being built when relying
on an implicit `BUILD_TESTING=ON` setting, and ending up with a link
error when building the `testdeps` target.

It did not cause issues when `BUILD_TESTING` was explicitly set.

Move the initialization before the first use to fix it.

Regression from aace27b #12287
Closes #13668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants