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

Fixes #14166: Add strip option to cmake.install tool #14167

Merged
merged 4 commits into from
Jul 6, 2023

Conversation

sagi-ottopia
Copy link
Contributor

@sagi-ottopia sagi-ottopia commented Jun 25, 2023

Changelog: Feature: Add new tools.cmake:install_strip conf to add --strip option to cmake install.
Docs: conan-io/docs#3281

Fix: #14166

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@CLAassistant
Copy link

CLAassistant commented Jun 25, 2023

CLA assistant check
All committers have signed the CLA.

@sagi-ottopia sagi-ottopia changed the base branch from release/2.0 to develop June 25, 2023 17:58
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

as the CMake is a wrapper around the command line I see no strong reason against this feature and this PR. However, this approach requires modifying recipes, maybe it would make sense to have a conf like tools.cmake.install:strip=xxx that controls that behavior, so it is possible to do it at scale for the whole dependency graph?

For this PR, it would be great if you could add some unit test that covers the behavior. I think that a unittest could be enough for this one, please have a look, and if you need help, let us know, we can contribute the test. Thanks again!

@sagi-ottopia
Copy link
Contributor Author

Hi,
Regarding a conf value, I think it should be easy to add it to the PR as well. I will look for some example and will update the PR accordingly.

Regarding unittests, sure.

@memsharded
Copy link
Member

Regarding a conf value, I think it should be easy to add it to the PR as well. I will look for some example and will update the PR accordingly.

Only if it makes sense as a use case, is it something that you will prefer over modifying the recipes? Is it something that makes sense to do for all packages in the graph or is it something specific to some?

I am adding @jcar87 for some feedback about this use case too.

@sagi-ottopia
Copy link
Contributor Author

Regarding a conf value, I think it should be easy to add it to the PR as well. I will look for some example and will update the PR accordingly.

Only if it makes sense as a use case, is it something that you will prefer over modifying the recipes? Is it something that makes sense to do for all packages in the graph or is it something specific to some?

I am adding @jcar87 for some feedback about this use case too.

I think it makes sense,
because when using the a conf value I can add it just in my Release builds profile without the need to have something like the following in my conanfile.py:

do_strip = False
if self.settings.build_type == 'Release':
   do_strip = True
...
cmake.install(strip=do_strip)

@memsharded
Copy link
Member

if self.settings.build_type == 'Release':

Ok, makes sense, though nowadays, I'd expect that Release builds already don't contain any debug information to be stripped, I understand it might not be the case for all platforms and tools, so I am ok with making this a opt-in conf.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looks great, many thanks!

conan/tools/cmake/cmake.py Outdated Show resolved Hide resolved
conans/test/unittests/tools/cmake/test_cmake_install.py Outdated Show resolved Hide resolved
sagi-ottopia and others added 2 commits July 5, 2023 19:18
Co-authored-by: James <memsharded@gmail.com>
Co-authored-by: James <memsharded@gmail.com>
@sagi-ottopia
Copy link
Contributor Author

Hi @memsharded,
saw that you added this into 2.0.8 timeline,
is it possible to merge it also to 1.XX latest version ?
Thanks.

@memsharded memsharded modified the milestones: 2.0.8, 1.61 Jul 5, 2023
@memsharded
Copy link
Member

Yes, sorry, didn't check the target branch. Moved it to 1.61, and then I will merge it myself to 2.0 branch (2.0.8 will probably be out earlier than 1.61)

Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

if self.settings.build_type == 'Release':

Ok, makes sense, though nowadays, I'd expect that Release builds already don't contain any debug information to be stripped, I understand it might not be the case for all platforms and tools, so I am ok with making this a opt-in conf.

From https://discourse.cmake.org/t/what-does-strip-do/4302 and if I'm not mistaken, it's not only debug information that gets stripped (if there is any), but also symbols that don't need to be externally visible, so this should have a benefit.

PR looks good to me, thanks @sagi-ottopia for your contribution!!!

@memsharded one thing I'd consider outside of this PR is adding a cli_args argument to cmake.install() - to mirror what we have for cmake.configure() and cmake.build() - while the current setup for install pretty much covers the bases, it may be useful for some advanced users to have the added flexibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Add '--strip' option to cmake install
4 participants