latchset / jose

C-language implementation of Javascript Object Signing and Encryption
Apache License 2.0
179 stars 49 forks source link

Update meson.build #153

Closed hdholm closed 6 months ago

hdholm commented 8 months ago

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

sarroutbi commented 7 months ago

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

simo5 commented 7 months ago

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

hdholm commented 7 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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

hdholm commented 6 months ago

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 commented 6 months ago

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

sarroutbi commented 6 months ago

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