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

[open62541] Add OpenSSL encryption, update Julia compat and increase version number (1.4.0) #7889

Merged
merged 22 commits into from
Apr 18, 2024

Conversation

thomvet
Copy link
Contributor

@thomvet thomvet commented Jan 2, 2024

No description provided.

@imciner2
Copy link
Member

imciner2 commented Jan 2, 2024

You should declare a dependency on MbedTLS_jll since you want to start using the mbedtls library. Is there a specific version that is needed?

Also, @giordano is mbedtls still a stdlib where version compats have to be kept in line with Julia versions?

@thomvet
Copy link
Contributor Author

thomvet commented Jan 2, 2024

How would I check this? The open62541 docs just use apt get install on Debian to install libmedtls (https://www.open62541.org/doc/1.3/building.html).

Checking debian pages anything above 2.28.3 should be fine (counting from stable release onwards), see here: https://packages.debian.org/search?keywords=mbedtls

O/open62541/build_tarballs.jl Outdated Show resolved Hide resolved
thomvet and others added 2 commits January 2, 2024 16:03
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@thomvet thomvet changed the title [open62541] Add TLS encryption to v1.3.9 build [open62541] Add TLS encryption, update Julia compat and increase version number Jan 2, 2024
@thomvet thomvet changed the title [open62541] Add TLS encryption, update Julia compat and increase version number [open62541] Add TLS encryption, update Julia compat and increase version number (1.3.10) Jan 2, 2024
@thomvet
Copy link
Contributor Author

thomvet commented Jan 2, 2024

I hope patch version increase is fine for this. If not (and v1.4.0 would be the one to do), then I will wait for the open62541 v1.4.0 release that should happen in a month or so and keep numbers in sync like this.

@giordano
Copy link
Member

giordano commented Jan 2, 2024

then I will wait for the open62541 v1.4.0 release that should happen in a month or so and keep numbers in sync like this.

This may be a nicer option, since the new release isn't too far away

@thomvet thomvet changed the title [open62541] Add TLS encryption, update Julia compat and increase version number (1.3.10) [WIP] [open62541] Add TLS encryption, update Julia compat and increase version number (1.3.10) Jan 2, 2024
@thomvet
Copy link
Contributor Author

thomvet commented Jan 2, 2024

I agree; changed title to WIP and will update with correct commit hash once the release is out.

@imciner2
Copy link
Member

imciner2 commented Apr 2, 2024

There has been a large effort recently to move away from MbedTLS and to OpenSSL instead. Can open62541 use OpenSSL?

@thomvet
Copy link
Contributor Author

thomvet commented Apr 3, 2024

Both is possible (in the sense of "either or"), as detailed here: https://www.open62541.org/doc/master/building.html

Maybe it would be helpful to feed the arguments for either MBEDTLS or OPENSSL here or add a cross reference to relevant discussions?

I was for example able to find this: JuliaLang/julia#48799

@imciner2
Copy link
Member

imciner2 commented Apr 3, 2024

The effort has mainly started because the upstream MbedTLS seems to not publish their plans for future LTS versions (so Julia doesn't know when they are coming), and also can have ABI compatibility issues between releases. With the effort going into switching the stdlibs that ship with Julia from MbedTLS to OpenSSL (#8386, #8377), that means OpenSSL will already be a necessary part of Julia and so won't require the user getting another dependency.

@fxcoudert and @giordano can probably explain more though.

@thomvet
Copy link
Contributor Author

thomvet commented Apr 3, 2024

These sound like very valid reasons to me and in the end I just wanted to provide potential (future) users of open62541.jl with the possibility to have encryption activated - it's of course nicer if it's aligned with the general direction that Julia is taking in this matter!

There's some time before a decision is "necessary", because the 1.4 release of open62541 is taking a little bit more time than expected and, as agreed above, I will aim to have this PR done only after that release happened.

@thomvet thomvet changed the title [WIP] [open62541] Add TLS encryption, update Julia compat and increase version number (1.3.10) [open62541] Add OpenSSL encryption, update Julia compat and increase version number (1.4.0) Apr 15, 2024
@thomvet
Copy link
Contributor Author

thomvet commented Apr 15, 2024

open62541 has released version 1.4.0 a couple of days ago, but I can't seem to compile it with -DUA_ENABLE_AMALGAMATION=ON on any platform (with or without OpenSSL included). Error message simply saying that it can't make install with that flag on (previously worked, though).

If I set the flag to OFF (d3343e9), then it builds on most platforms (but it would mean that I need to alter my Clang.jl generator script for my wrapper package?), but not all.

On the platforms where it fails to build:
x86_64-unknown-freebsd:

[08:44:03] /workspace/srcdir/open62541/arch/eventloop_posix.c:551:46: error: use of undeclared identifier 'SO_REUSEPORT'
 [08:44:03]     res \|= UA_setsockopt(sockfd, SOL_SOCKET, SO_REUSEPORT,
 [08:44:03]                                              ^
 [08:44:03] 1 error generated.
 [08:44:03] make[2]: *** [CMakeFiles/open62541-plugins.dir/build.make:446: CMakeFiles/open62541-plugins.dir/arch/eventloop_posix.c.o] Error 1
 [08:44:03] make[2]: *** Waiting for unfinished jobs....

x86_64-w64-mingw32:

[08:43:53] CMake Error at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
[08:43:53]   Could NOT find OpenSSL, try to set the path to OpenSSL root folder in the
[08:43:53]   system variable OPENSSL_ROOT_DIR (missing: OPENSSL_CRYPTO_LIBRARY) (found
[08:43:53]   version "3.0.8")
[08:43:53] Call Stack (most recent call first):
[08:43:53]   /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
[08:43:53]   /usr/share/cmake/Modules/FindOpenSSL.cmake:574 (find_package_handle_standard_args)
[08:43:53]   CMakeLists.txt:514 (find_package)

Maybe one of the wizards around here can help me?

Obviously, I probably could get x86_64-w64-mingw32 work without OpenSSL included in the library and I would have to live with updating my Clang.jl generator script in that case. My preferred solution would of course be to have -DUA_ENABLE_AMALGAMATION=ON and the build passing on all systems...

@giordano
Copy link
Member

Error message simply saying that it can't make install with that flag on (previously worked, though).

It's less weird if you git blame the relevant line and see that the error was introduced three weeks ago: open62541/open62541#6369. It looks very deliberate.

@thomvet
Copy link
Contributor Author

thomvet commented Apr 15, 2024

Thanks for finding that discussion. I think I understand the reasoning behind the change and it makes sense to me (just means that I have to update the generator script for my wrapper - that's an "OK" bit of work and it seems I can't avoid it :) ).

@giordano
Copy link
Member

x86_64-unknown-freebsd:

I haven't tested it locally, but according to the FreeBSD setsockopt manual, that requires

#include	<sys/types.h>
#include	<sys/socket.h>

but looking at https://github.com/open62541/open62541/blob/8be32a09479401015bd8196f1c21bfdfe1849ab4/arch/eventloop_posix/eventloop_posix.h they only include sys/socket.h, this may be an upstream bug.

x86_64-w64-mingw32:

# Necessary for cmake to find openssl on Windows
if [[ ${target} == x86_64-*-mingw* ]]; then
export OPENSSL_ROOT_DIR=${prefix}/lib64
fi

@thomvet
Copy link
Contributor Author

thomvet commented Apr 16, 2024

I opened up an issue: open62541/open62541#6414
Let's see what input we are getting.

@thomvet
Copy link
Contributor Author

thomvet commented Apr 17, 2024

As per the issue linked above, this is indeed an upstream bug.
I am wondering on how to proceed here. I see several options:

  1. Generate a open62541_jll version with open62541 v1.4.0 excluding FreeBSD support, then re-add FreeBSD support once fixed.
  2. Wait for open62541 v1.4.x (assuming a PR is accepted that corrects the issue) and then build on this.
  3. Find a workaround for the bug, somehow patching the FreeBSD build of open62541 v1.4.0. Don't know whether it's possible or how to do it.

Option 1 would allow me to start working on the necessary changes to reflect the separate header files rather than the amalgamated version in the wrapper package (https://github.com/martinkosch/open62541.jl).

@giordano
Copy link
Member

Option 3 would be best, but we need a patch to apply to the source of v1.4.0.

@thomvet
Copy link
Contributor Author

thomvet commented Apr 18, 2024

Ok, I think the patch worked (sorry, first had to actually understand how to make one - first time patching something!); the library now builds again on all platforms.

@giordano, I think this is now ready for another look by you.

O/open62541/build_tarballs.jl Outdated Show resolved Hide resolved
@giordano giordano merged commit d88cdf9 into JuliaPackaging:master Apr 18, 2024
19 checks passed
@thomvet
Copy link
Contributor Author

thomvet commented Apr 19, 2024

Thank you for all your help on this!

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.

3 participants