jose icon indicating copy to clipboard operation
jose copied to clipboard

Update meson.build

Open hdholm opened this issue 1 year ago • 3 comments
trafficstars

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.)

hdholm avatar Mar 28 '24 01:03 hdholm

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

sarroutbi avatar Apr 08 '24 07:04 sarroutbi

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

simo5 avatar Apr 08 '24 14:04 simo5

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 https://github.com/latchset/jose/pull/153#pullrequestreview-1986698563, or unsubscribe https://github.com/notifications/unsubscribe-auth/APPKJWNYQLTNPEGDBGQBZ7LY4KV2BAVCNFSM6AAAAABFL5CGLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBWGY4TQNJWGM . You are receiving this because you were mentioned.Message ID: @.***>

hdholm avatar Apr 08 '24 18:04 hdholm

@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.

hdholm avatar May 18 '24 01:05 hdholm

@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 avatar May 18 '24 08:05 sarroutbi

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

sarroutbi avatar May 18 '24 08:05 sarroutbi

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.

hdholm avatar May 19 '24 01:05 hdholm

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

simo5 avatar May 20 '24 12:05 simo5

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 avatar May 22 '24 09:05 sarroutbi