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: Skip superfluous PIC/PIE warning for Windows #58

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Nov 29, 2023

PIC/PIE is not a thing in Windows PE format. Therefore, warnings are rather making noise than providing valuable feedback.

@hebasto
Copy link
Owner Author

hebasto commented Dec 6, 2023

Rebased.

@theuni
Copy link

theuni commented Dec 6, 2023

It's frustrating that CMake doesn't just know this. It's also frustrating that check_pie_supported seems to just be wrong for Windows? I wonder if we should upstream a fix/change?

FWIW, pic/pie is default on for macOS, so if we're not already getting warnings there, we probably will at some point, so we may as well include that here as well.

@fanquake
Copy link

fanquake commented Dec 6, 2023

FWIW, pic/pie is default on for macOS,

Was this also always the case for x86_64? I guess starting with arm64, it was just made impossible to disable.

@hebasto
Copy link
Owner Author

hebasto commented Dec 6, 2023

It's frustrating that CMake doesn't just know this. It's also frustrating that check_pie_supported seems to just be wrong for Windows? I wonder if we should upstream a fix/change?

Sorry if I understood your comment in a wrong way, but it seems that CMake does its job well. On Windows, the check_pie_supported function clearly recognizes that "PIE and NO_PIE are not supported by linker for CXX".

It was our decision to print a warning regardless of its usefulness.

If guarding the whole code block with if(NOT WIN32) looks unclear, maybe do it for message(WARNING ...) only?

@TheCharlatan
Copy link

If guarding the whole code block with if(NOT WIN32) looks unclear, maybe do it for message(WARNING ...) only?

That seems preferable to me, maybe also with a small comment explaining why?

@hebasto hebasto changed the title cmake: Skip PIC/PIE check and warning for Windows cmake: Skip superfluous PIC/PIE warning for Windows Dec 11, 2023
PIC/PIE is not a thing in Windows PE format. Therefore, warnings are
rather making noise than providing valuable feedback.
@hebasto
Copy link
Owner Author

hebasto commented Dec 11, 2023

Rebased.

Addressed @TheCharlatan's comment.

Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK 52447c3

@hebasto hebasto merged commit 8ba1ed4 into cmake-staging Dec 12, 2023
6 checks passed
@theuni
Copy link

theuni commented Dec 14, 2023

Post-merge ACK. Thanks for the explanation, @hebasto, I had indeed misunderstood.

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.

4 participants