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: fix error getting LOCATION property on non-imported target #7885

Closed
wants to merge 1 commit into from
Closed

cmake: fix error getting LOCATION property on non-imported target #7885

wants to merge 1 commit into from

Conversation

@Boris-Rasin
Copy link
Contributor

@Boris-Rasin Boris-Rasin commented Oct 20, 2021

CMake allows getting LOCATION property only on IMPORTED targets:
https://cmake.org/cmake/help/latest/prop_tgt/LOCATION.html

Currently, curl cmake avoids getting LOCATION property on INTERFACE targets only.
With this change, it will avoid getting LOCATION property on all non-imported targets, not only INTERFACE targets.

This is required when both curl and zlib are built in the same CMake project (for example, when build with CMake package manager: https://github.com/scapix-com/cmodule), in which case ZLIB::ZLIB is a normal CMake target (not IMPORTED and not INTERFACE).

@bagder bagder added the cmake label Oct 20, 2021
@Boris-Rasin
Copy link
Contributor Author

@Boris-Rasin Boris-Rasin commented Oct 26, 2021

Any chance this will be accepted?
This doesn't change anything for cases which currently work (IMPORTED or INTERFACE targets).
This only changes behavior for cases which currently fail (regular targets, not IMPORTED and not INTERFACE).

Loading

@bagder bagder requested a review from jzakrzewski Oct 28, 2021
@bagder
Copy link
Member

@bagder bagder commented Oct 28, 2021

We just need someone who knows something about cmake to approve it!

Loading

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Oct 28, 2021

The docu suggests that it can be used for not IMPORTED targets but simply reading the description gives me headache. It's a mess that can go wrong quite easily and is basically only provided for compatibility, so I think this PR is a good change.

Loading

@bagder
Copy link
Member

@bagder bagder commented Oct 28, 2021

Thanks, both of you!

Loading

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

Successfully merging this pull request may close these issues.

None yet

3 participants