-
Notifications
You must be signed in to change notification settings - Fork 50
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
Update meson.build #153
Conversation
Add undefined-version flag to link check for version-script to avoid false failures do to the "code" in the check being trivial.
Remove extraneous space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM
@sergio-correia : may I ask you to double check this change? It LGTM, but I would like to have a second opinion |
This will allow unversioned symbols to creep through .. sounds wrong. |
There was a problem hiding this 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 ...
Will it really allow unversioned symbols? This is only added to the
compile check, not to the actual compile or link, or did I miss something?
…On Mon, Apr 8, 2024 at 10:56 AM Sergio Arroutbi ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hello @hdholm <https://github.com/hdholm> . May I ask this change to be
exclusive for architecture affected? It seems this is wrong in other cases
...
—
Reply to this email directly, view it on GitHub
<#153 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/APPKJWNYQLTNPEGDBGQBZ7LY4KV2BAVCNFSM6AAAAABFL5CGLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBWGY4TQNJWGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@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 |
Change LGTM. Anything to add, @sergio-correia, @simo5? I think it would be great to include this in the next release |
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. |
@hdholm sorry misread the initial change, given this is only a setup time heck I see no problem. |
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 |
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.)