Skip to content

OpenSSL only if feature networking is set.#142

Merged
jbaldwin merged 4 commits intojbaldwin:mainfrom
dok-net:openssl_only_feature_networking
Jun 4, 2023
Merged

OpenSSL only if feature networking is set.#142
jbaldwin merged 4 commits intojbaldwin:mainfrom
dok-net:openssl_only_feature_networking

Conversation

@dok-net
Copy link
Contributor

@dok-net dok-net commented Jun 2, 2023

This fixes #141 to not include OpenSSL if there's no networking selected.

@dok-net dok-net force-pushed the openssl_only_feature_networking branch from 173d1d0 to 5c60c76 Compare June 2, 2023 23:50
@dok-net
Copy link
Contributor Author

dok-net commented Jun 2, 2023

This alone doesn't turn off bulding the tests in the submodule vendor/tartanllama/expected, which takes a very long time. Any ideas?

mkdir Release && cd Release
cmake -DCMAKE_BUILD_TYPE=Release -DLIBCORO_FEATURE_NETWORKING=OFF -DLIBCORO_BUILD_TESTS=OFF ..
cmake --build .

@jbaldwin
Copy link
Owner

jbaldwin commented Jun 2, 2023

Ah yeah I missed this, thanks for opening a PR. I'll need to think on the tests, but maybe that part can just be disabled as well, I think it's just making a unique SSL cert per run.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2023

@jbaldwin I've added the actual cmake line I was using, I've disabled tests anyway. See how the expected lib builds their tests, anyway. What about C++23 previews, can the expected submodule get ignored for those already, and if, how?

@jbaldwin
Copy link
Owner

jbaldwin commented Jun 3, 2023

Can you open an issue for the expected lib building tests when tests are disabled? Its a bug in the build system for sure, it might affect the other vendor deps as well

@jbaldwin
Copy link
Owner

jbaldwin commented Jun 3, 2023

We can use the expected vendor for c++20 and for c++23 use the stdlib version, its possible via cmake to detect the c++ version and only include the vendor one if its < c++23 basically. I would make that as a different issue as well. Realistically I think targeting c++20 for this library indefinitely makes the most sense though, but lets pickup new features if the user is using a new version!

@jbaldwin
Copy link
Owner

jbaldwin commented Jun 3, 2023

I'm not exactly sure I can add a commit to your fork's PR so I just opened another PR here with the additions to make the tests work properly:

#144

@jbaldwin jbaldwin self-assigned this Jun 3, 2023
@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2023

The cmake configure step still probes for all the networking related APIs.
Edit: This seems to no longer be the case after the latest changes to this PR.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2023

Can the c-ares be excluded, depending on the networking feature selection?

@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2023

I'm not exactly sure I can add a commit to your fork's PR so I just opened another PR here with the additions to make the tests work properly:

#144

I've included your changes into my PR branch.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2023

Lots of "catch-2" build steps still occur, though.

@jbaldwin
Copy link
Owner

jbaldwin commented Jun 3, 2023

Yes, I think that should be fixed on the other PR here #143, this is just removing openssl with LIBCORO_FEATURE_NETWORKING=OFF. Trying to keep them distinct commits since they are different issues.

@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2023

I'm not merging the pending PRs and pushing, just have it all at once in my work tree and it's not completely there, just yet.

@dok-net dok-net force-pushed the openssl_only_feature_networking branch from d6a26e8 to 0a26e51 Compare June 3, 2023 15:38
@dok-net
Copy link
Contributor Author

dok-net commented Jun 3, 2023

@jbaldwin This PR includes your branch origin/openssl_only_feature_networking completely, and finishes the remaining issue with "c-ares". Please review, it's ready to merge from my side.

@jbaldwin jbaldwin force-pushed the openssl_only_feature_networking branch from dd30e56 to d7748d8 Compare June 4, 2023 16:30
@jbaldwin jbaldwin merged commit 88dd833 into jbaldwin:main Jun 4, 2023
@dok-net dok-net deleted the openssl_only_feature_networking branch June 4, 2023 18:48
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.

2 participants