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

openssl: add asm support for windows-x86_x64/windows-x86 #1679

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Teselka
Copy link

@Teselka Teselka commented Sep 10, 2024

Adds support for the openssl's assembler files by using GNU/Netwide Assembler on Windows.
One bad thing is that it generates WARNING: Project targets '>= 0.55' but uses feature introduced in '0.64.0': Adding NASM language. when configuring the project.
Not sure what to do with this one, should i bump the minimum required version?

@eli-schwartz
Copy link
Member

@nazar-pc

@benoit-pierre
Copy link
Contributor

benoit-pierre commented Sep 10, 2024

For the warning: you can update the initial project(…) call like this.

@bgilbert
Copy link
Contributor

For the warning: you can update the initial project(…) call like this.

Isn't that structure redundant though? Meson won't warn about properly version-gated constructs even if the version in the project() call is lower.

Comment on lines +133 to +140
if has_win_asm
if is_x86
arch_subdir = 'VC-WIN32'
elif is_x86_64
arch_subdir = 'VC-WIN64A'
else
asm = 'no-asm'
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably keep warning(…) if ASM support is disabled on x86 / x86_64.

Copy link
Author

Choose a reason for hiding this comment

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

Sure? Another arch/os combos doesn't have such warning

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know… There's no summary(…) call at the end, but there's a message, not a warning, in case of no-asm… ¯\(ツ)

Copy link
Member

Choose a reason for hiding this comment

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

If someone disables a feature via an option we don't need to warn them:

  • that the build system is incapable of offering that feature for their configuration
  • that the build system is capable, but they have turned it off (they know they turned it off)

If something is optional and auto detected at configure time, then is the right time to warn people that functionality which they might have assumed would be built, is not being built.

Copy link
Member

Choose a reason for hiding this comment

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

A summary might be apropos. Those don't abort when using --fatal-meson-warnings.

@eli-schwartz
Copy link
Member

Not sure what to do with this one, should i bump the minimum required version?

People on Windows who end up running the code in the if certainly need that new meson version, in which case bumping the minimum is apropos.

But Linux users don't need it. Is it worth bumping a minimum over this? I don't know.

If you do not bump the minimum then you'll need to guard that logic using if meson.version().version_compare('>=0.64.0') and gracefully degrade to not provide those features to windows users running old meson. No warning will be issued for code behind such a guard.

@benoit-pierre
Copy link
Contributor

benoit-pierre commented Sep 10, 2024

For the warning: you can update the initial project(…) call like this.

Isn't that structure redundant though? Meson won't warn about properly version-gated constructs even if the version in the project() call is lower.

That's not my understanding: if the project call ask for >= 0.55, but meson is 0.64 and a feature only supported on >= 0.64 is used, then there will be a warning, no?

@eli-schwartz
Copy link
Member

For the warning: you can update the initial project(…) call like this.

That... Uh. Looks kind of dangerous 👀 because it erases warnings for code not in a version guard, on top of version guards will already suppress warnings when detected.

@benoit-pierre
Copy link
Contributor

Well, it definitely does not work with curl in this case.

@Teselka
Copy link
Author

Teselka commented Sep 10, 2024

Suppressed this warning properly i guess

has_win_asm = add_languages(meson.version().version_compare('>=0.64') ? 'nasm' : '', native: false, required: false)

@eli-schwartz
Copy link
Member

Please use if/endif conditionals, not ternary argument passing.

Well, it definitely does not work with curl in this case.

Meson's version guard handling is implemented by introducing a context inside the body of an if/endif. In your case, you are building dictionaries of kwargs but the actual call site that uses it isn't guarded by a conditional. It might still be possible to detect this by tagging the kwargs dictionary with an attribute somehow, but no one has designed an implementation of such a thing...

I understand why you did it the way you did (repeating the function twice isn't very DRY and this one is particularly bulky) and I cannot say I'd have done differently in your shoes, but nonetheless... mechanically, that's not what meson recognizes as a version guard. :(

@benoit-pierre
Copy link
Contributor

Please use if/endif conditionals, not ternary argument passing.

Well, it definitely does not work with curl in this case.

Meson's version guard handling is implemented by introducing a context inside the body of an if/endif. In your case, you are building dictionaries of kwargs but the actual call site that uses it isn't guarded by a conditional. It might still be possible to detect this by tagging the kwargs dictionary with an attribute somehow, but no one has designed an implementation of such a thing...

I understand why you did it the way you did (repeating the function twice isn't very DRY and this one is particularly bulky) and I cannot say I'd have done differently in your shoes, but nonetheless... mechanically, that's not what meson recognizes as a version guard. :(

Makes sense, cleaned up in #1680.

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