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: Set POSITION_INDEPENDENT_CODE property properly #1232

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 10, 2023

This PR is an alternative to #1230.

Benefits of this PR in comparison to #1230:

  • keeps ability to build a shared library and a static one simultaneously, which is convenient, for example, during a process of creating a distributable package like libsecp256k1-dev using CPack tool
  • has smaller diff
  • does not require CMake version bumping

Now, a user can set PIC/non-PIC mode for a static library using CMAKE_POSITION_INDEPENDENT_CODE, and this setting is independent from a shared library.

@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2023

cc @theuni

@theuni
Copy link
Contributor

theuni commented Mar 10, 2023

Concept NACK for the reasons described in this thread: #1113 (comment). Though take that for what it's worth given my obvious bias from #1230.

  • keeps ability to build a shared library and a static one simultaneously, which is convenient

I won't disagree this is convenient, but I don't agree that it's worth the complexity.

  • has smaller diff

I don't think that's relevant here as I believe all reviewers of #1113 assumed we'd be making some structural changes in a follow-up. If I thought this was locked in I wouldn't have ACKed.

  • does not require CMake version bumping

@real-or-random doesn't seem too concerned about this (quite the opposite actually).

I guess this is really just a question for maintainers. I'm not opposed to simultaneous libs, I just don't think it's very idiomatic or worth the trouble.

I'm also not trying to keep bumping up the relevance of something so trivial as PIC. I suspect the reason it keeps coming up is because it's an example of something that's correctly handled (technically anyway, style aside) rather simply in a one-at-a-time build, but gets complex with simultaneous builds..

@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2023

Concept NACK for the reasons described in this thread: #1113 (comment). Though take that for what it's worth given my obvious bias from #1230.

  • keeps ability to build a shared library and a static one simultaneously, which is convenient

I won't disagree this is convenient, but I don't agree that it's worth the complexity.

If we follow #1230, what packaging process at some point in the future can be imagined? Run cmake / cmake --build procedure twice? Isn't it another type of complexity?

@real-or-random
Copy link
Contributor

I tend to think that not setting PIC globally is a bit closer to the philosophy we followed so far, where we let the user decide as much as possible. This is just because the goal is to target a broad range of platforms and thus want to avoid making assumptions about our users as much as possible (and maybe that part is also biased/inspired by the philosophy of autotools). I don't think I'm very competent to judge how relevant it is to let the user decide in the specific case of PIC. But if you ask me, I'd also lean towards #1230.


If we follow #1230, what packaging process at some point in the future can be imagined? Run cmake / cmake --build procedure twice? Isn't it another type of complexity?

To be honest, I'm convinced that we'll ever ship binaries. So, it probably won't be relevant to us. (And if it is, it would occur about every three months, so it's not a big deal to run things twice.) As for others packaging our library, I think they'll anyway be only interested in the shared library in 99% of the cases.

@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2023

Closing in favour of #1230.

@hebasto hebasto closed this Mar 10, 2023
@hebasto hebasto deleted the 230309-pic branch April 29, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants