libressl / portable

LibreSSL Portable itself. This includes the build scaffold and compatibility layer that builds portable LibreSSL from the OpenBSD source code. Pull requests or patches sent to tech@openbsd.org are welcome.
https://www.libressl.org
1.35k stars 269 forks source link

Mop up #1084 breakage #1100

Closed botovq closed 2 days ago

botovq commented 2 days ago

Revert the minimum cmake version change since that isn't viable for major linux distros. Also, it led to weird compilation failures due to some undesirable side effects when compiling assembly files:

/__w/portable/portable/crypto/aes/vpaes-elf-x86_64.S:8:1: error: expected identifier or '(' before '.' token
    8 | .text
      | ^
vszakats commented 1 day ago

@botovq Thanks for the quick update!

Are you sure the if() works inside argument lists as expected?

This isn't documented (AFAICF) and compiles silently by CMake, but doesn't actually seem to enable the thing enclosed inside the condition. It also breaks when swapping the condition to, say, UNIX, or when adding an else() branch.

There is a way to add options with inline conditions in certain cases, with a different syntax (haven't tried this): https://cmake.org/cmake/help/v3.0/manual/cmake-generator-expressions.7.html

Or with additional calls to set_target_properties(ssl PROPERTIES ...). Or with set_property(TARGET ssl APPEND PROPERTY ...).

botovq commented 1 day ago

Are you sure the if() works inside arguments lists as expected?

No, of course not :)

Now that you point it out, I see the problem...

I don't have access to a Windows machine to check. Does #1103 work?

vszakats commented 1 day ago

@botovq #1103 LGTM, thanks! I also don't have a Windows machine to test, and my cross-builds are without DLLs. Reproducing a fitting build locally would be a bit tedious.

Within LibreSSL's Windows CI workflow, maybe adding a post-build step doing:

find . -name '*.exe' -o -name '*.dll'

would add a way to verify this in the logs. https://github.com/curl/curl/blob/0acf0bcedad9208703cdbe91b06cacb1065a6e53/.github/workflows/windows.yml#L162

botovq commented 1 day ago

@vszakats There's an upload build artifact stage in our Windows CI. This one from #1103 shows that the major is actually appended:

$ find . -name '*.dll'
./crypto/Release/crypto-55.dll
./ssl/Release/ssl-58.dll
./tests/crypto-55.dll
./tests/ssl-58.dll
./tests/tls-31.dll
./tls/Release/tls-31.dll

whereas on HEAD the majors are absent.

Thanks for the pointers!

https://github.com/libressl/portable/actions/runs/11142580203/artifacts/2005442115

vszakats commented 1 day ago

@botovq Nice, that also does the job! It's looking good.

busterb commented 1 day ago

Good catch @botovq, sorry about that. I'm a little baffled how changing the minimum CMake check could actually impact how assembly code is generated though.

botovq commented 1 day ago

@busterb I suspect it has to do with implicitly enabling more cmake policies. Not sure what it could be though.

botovq commented 1 day ago

@busterb It compiles the assembly files with cc -x c, which of course cannot work. I suspect it has to do with https://cmake.org/cmake/help/latest/policy/CMP0056.html

Not sure how to fix this. Presumably we need to label the .S files as assembly files rather than C files somehow.

botovq commented 1 day ago

@busterb It's actually CMP0119 and we should probably look into using enable_language(ASM) rather than forcing the use of the C-compiler for some of the .S files.