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

Update meson.build #153

Merged
merged 3 commits into from
May 22, 2024
Merged

Update meson.build #153

merged 3 commits into from
May 22, 2024

Conversation

hdholm
Copy link
Contributor

@hdholm hdholm commented Mar 28, 2024

Add undefined-version flag to link check for version-script to avoid false failures do to the "code" in the check being trivial. This seems to resolve the problem described in Issue #152 and https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=277905 without adversely affecting any other builds (including OSX.)

Add undefined-version flag to link check for version-script to avoid false failures do to the "code" in the check being trivial.
lib/meson.build Outdated Show resolved Hide resolved
Remove extraneous space
Copy link
Collaborator

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Change LGTM

@sarroutbi
Copy link
Collaborator

@sergio-correia : may I ask you to double check this change? It LGTM, but I would like to have a second opinion

@simo5
Copy link
Member

simo5 commented Apr 8, 2024

This will allow unversioned symbols to creep through .. sounds wrong.

Copy link
Collaborator

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Hello @hdholm . May I ask this change to be exclusive for architecture affected? It seems this is wrong in other cases ...

@hdholm
Copy link
Contributor Author

hdholm commented Apr 8, 2024 via email

@hdholm
Copy link
Contributor Author

hdholm commented May 18, 2024

@sarroutbi @simo5 Is there something I'm missing in my previous comment? I don't think this has any effect on symbols in the actual built code.

@sarroutbi
Copy link
Collaborator

@sarroutbi @simo5 Is there something I'm missing in my previous comment? I don't think this has any effect on symbols in the actual built code.

Hello @hdholm. I was wondering if this change could be applied only to FreeBSD compilation, as this seems to be something not affecting the rest of operating systems. I would prefer it, sincerely, to avoid changing flags on non affected distributions

@sarroutbi
Copy link
Collaborator

sarroutbi commented May 18, 2024

Change LGTM. Anything to add, @sergio-correia, @simo5? I think it would be great to include this in the next release

@hdholm
Copy link
Contributor Author

hdholm commented May 19, 2024

Hello @hdholm. I was wondering if this change could be applied only to FreeBSD compilation, as this seems to be something not affecting the rest of operating systems. I would prefer it, sincerely, to avoid changing flags on non affected distributions

I could, but I think that actually adds more complexity than leaving it this way. I also suspect, although I'm not positive, that other architectures could have the same issue with newer linkers that seem to care that there are no actual symbols to reference in the code being fed to the check, which is really just checking for the existence of a linker flag assuming I understand the code correctly. The code here seems to correctly separate the linkers that need different flags (looks like essentially MacOS) from those that don't without any errors in any of the architectures.

@simo5
Copy link
Member

simo5 commented May 20, 2024

@hdholm sorry misread the initial change, given this is only a setup time heck I see no problem.

@sarroutbi
Copy link
Collaborator

Hello @hdholm. I was wondering if this change could be applied only to FreeBSD compilation, as this seems to be something not affecting the rest of operating systems. I would prefer it, sincerely, to avoid changing flags on non affected distributions

I could, but I think that actually adds more complexity than leaving it this way. I also suspect, although I'm not positive, that other architectures could have the same issue with newer linkers that seem to care that there are no actual symbols to reference in the code being fed to the check, which is really just checking for the existence of a linker flag assuming I understand the code correctly. The code here seems to correctly separate the linkers that need different flags (looks like essentially MacOS) from those that don't without any errors in any of the architectures.

Hello @hdholm . We will merge it this way. If, in the future, other architectures are similarly impacted, we will apply it to all of them. Thanks for your contribution

@sarroutbi sarroutbi merged commit 3f11058 into latchset:master May 22, 2024
22 checks passed
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