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

Let's give a cmake try to build per tools #2

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Sashan
Copy link
Contributor

@Sashan Sashan commented Jul 13, 2024

The change also removes usused variables I've found occasionally.

To build performance tools on windows we must add missing pieces:
    - getopt()
    - basename()
    - strcasecmp()
all thos implementations are homebrewed

Let cmake to determine openssl version we use to build tools.

In order to tell CMake which OpenSSL version to use we use
env. variabe OPENSSL_ROOT_DIR which points to OpenSSL installation
prefix with desired version.

We build evp_fetch and providerdoall test tools for OpenSSL 3.*.
We don't build those for 1.1.1.

CMake seems to work fine on windows and OpenBSD.

The OpenBSD binaries produced by build process use RPATH. There is no
such thing on Windows. On windows you need to add your OPENSSL_ROOT_DIR\bin
to your PATH. Otherwise the .exe file produced by build process won't work.

@nhorman
Copy link
Contributor

nhorman commented Jul 13, 2024

the changes look reasonable, but you have a fixup as your first commit, not sure that will autosquash properly

@Sashan
Copy link
Contributor Author

Sashan commented Jul 13, 2024

I've just force pushed repository with squashed changes.

bonus: it builds on MacOS too. Gave a try 1.1.1, 3.0 3.3 and 3.4-dev everything worked fine.

source/CMakeLists.txt Outdated Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Jul 17, 2024

I did a bit of hacking, and a much bigger instruction comment in source/CMakeLists.txt. Mind of I simply push a fixup commit into this PR?

@levitte
Copy link
Member

levitte commented Jul 17, 2024

This got me to test the OpenSSLConfig.cmake we produce for using an OpenSSL build without installing it, and oh boy, I found a bug (fixed in openssl/openssl#24918)

@Sashan
Copy link
Contributor Author

Sashan commented Jul 17, 2024

I did a bit of hacking, and a much bigger instruction comment in source/CMakeLists.txt. Mind of I simply push a fixup commit into this PR?

please, go ahead and push your changes in.

@levitte
Copy link
Member

levitte commented Jul 17, 2024

Done. Don't forget to pull...

source/CMakeLists.txt Outdated Show resolved Hide resolved
source/CMakeLists.txt Outdated Show resolved Hide resolved
source/CMakeLists.txt Outdated Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Jul 17, 2024

BTW, if you want to see what's actually happening when building:

$ cmake --build ./build --verbose

@Sashan
Copy link
Contributor Author

Sashan commented Aug 21, 2024

update CMakeList.txt so it can build evp_setpeer which has just landed to repository.
I'm also doing a minor tweak to evp_setpeer.c to fix build on windows.

source/perflib/basename.c Outdated Show resolved Hide resolved
@Sashan
Copy link
Contributor Author

Sashan commented Aug 26, 2024

@levitte it would be nice to have those changes in so everyone can build tools on windows. thanks.

@levitte
Copy link
Member

levitte commented Aug 26, 2024

@levitte it would be nice to have those changes in so everyone can build tools on windows. thanks.

I agree. Might I encourage you to nudge others to review them?

@t8m t8m added triaged: feature The issue/pr requests/adds a feature approval: review pending This pull request needs review by a committer labels Aug 29, 2024
@t8m
Copy link
Member

t8m commented Aug 29, 2024

ping @openssl/committers for second review

@levitte
Copy link
Member

levitte commented Aug 30, 2024

I pushed a fixup just now, changing CMakeLists.txt a little bit.

This moves all the dependencies on OpenSSL to the perf library, and also declares its public include directory.

With that, all executables only need to depend on the perf library, and the magic of CMake will take care of the rest.

I've decided to use cmake to get it done.

The change also removes usused variables I've found occasionally.

To build performance tools on windows we must add missing pieces:
    - getopt()
    - basename()
    - strcasecmp()
all thos implementations are homebrewed

Let cmake to determine openssl version we use to build tools.

In order to tell CMake which OpenSSL version to use we use
env. variabe OPENSSL_ROOT_DIR which points to OpenSSL installation
prefix with desired version.

We build evp_fetch and providerdoall test tools for OpenSSL 3.*.
We don't build those for 1.1.1.
levitte and others added 2 commits August 30, 2024 16:41
This moves all the dependencies on OpenSSL to the perf library,
and also declares its public include directory.

With that, all executables only need to depend on the perf library,
and the magic of CMake will take care of the rest.
…ows.

- build getopt.c, basename.c, ... on windows only
…on Windows.

- PUBLIC was wrong, cmake -S . -B ... ends up with error on windows, using PRIVATE got me going
@Sashan
Copy link
Contributor Author

Sashan commented Aug 30, 2024

please hold your new approvals/merging the cmake does not work on windows yet.
this is the error I get when I try to build it

C:\Users\OpenSSL\work.openssl\perftools\source>cmake --build build-master-one
ninja: error: build.ninja:54: bad $-escape (literal $ must be written as $$)
  INCLUDES = -IC:\Users\OpenSSL\work.openssl\perftools\source\$(PROJECT_...
                                                              ^ near here

not sure if it is glitch in cmake or ninja on windows, this is triggered by addition of target_sources() I'm still investigating the issue. will continue on Sunday evening.

source/CMakeLists.txt Outdated Show resolved Hide resolved
… tools on Windows.

- $(PROJECT_SOURCE_DIR) vs, ${PROJECT_SOURCE_DIR} well spotted by Richard
…ormance tools on Windows.

- s/target_source/target_sources
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: review pending This pull request needs review by a committer triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants