Find-SASL cleanup #1405
Merged
Find-SASL cleanup #1405
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Tweak importing of libsasl2 for user requirements Tweaks the import of libsasl2 so that at import-time we don't *require* the pkg-config files, since the dynamic library's dep on libsasl should be sufficient to satisfy the runtime linking.
Build +test-cxx-driver to run a default mongo-cxx-driver using the local mongo-c-driver as the driver backend.
kevinAlbs
reviewed
Sep 15, 2023
kkloberdanz
approved these changes
Sep 15, 2023
| @@ -282,6 +282,10 @@ function(_generate_pkg_config_content out) | |||
| $<GENEX_EVAL:$<TARGET_PROPERTY:pkg_config_LIBS>> | |||
| $<REMOVE_DUPLICATES:$<TARGET_PROPERTY:INTERFACE_LINK_OPTIONS>> | |||
| ) | |||
|
|
|||
| # XXX: Could we define a genex that can transform the INTERFACE_LINK_LIBRARIES to a list of | |||
There was a problem hiding this comment.
Is the XXX: intended to be here, or is it a placeholder?
There was a problem hiding this comment.
That's a callout to future readers that may know better than I do, since the current way its handled is sub-optimal but not "broken" to warrant a "TODO" or "FIXME".
kevinAlbs
approved these changes
Sep 18, 2023
There was a problem hiding this comment.
Changes LGTM, though one task failure may need to be investigated:
The MinGW task appears to fail to locate libsasl2:
[2023/09/15 21:08:31.592] CMake Error at C:/Users/mci-exec/AppData/Local/mongo-c-driver/tmp.pTUuQRMADo/cmake-3.25.2-windows-x86_64/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
[2023/09/15 21:08:31.593] Could NOT find SASL2 (missing: SASL2_VERSION SASL2_INCLUDE_DIR
[2023/09/15 21:08:31.593] SASL2_LIBRARY) (Required is at least version "2.0")
Earthfile
Outdated
| alpine-test-env-base: | ||
| ARG --required version | ||
| FROM +alpine-base --version=$version | ||
| RUN apk add libsasl snappy pkgconfig |
There was a problem hiding this comment.
Suggested change
| RUN apk add libsasl snappy pkgconfig | |
| RUN apk add libsasl snappy |
Remove package config dependency? I assume this is no longer needed due to 0c76946. Comment applies to other package config dependencies in this file: pkgconf and pkg-config
| @@ -47,9 +49,17 @@ def __init__(self, *, suffix: str, target: str) -> None: | |||
| "amazon2", | |||
| ] | |||
|
|
|||
| # Other optoins: SSPI (Windows only), AUTO (not reliably test-able without more environments) | |||
There was a problem hiding this comment.
Suggested change
| # Other optoins: SSPI (Windows only), AUTO (not reliably test-able without more environments) | |
| # Other options: SSPI (Windows only), AUTO (not reliably test-able without more environments) |
vector-of-bool
added a commit
to vector-of-bool/mongo-c-driver
that referenced
this pull request
Nov 3, 2023
* Redefine the handling of SASL imports Tweak importing of libsasl2 for user requirements Tweaks the import of libsasl2 so that at import-time we don't *require* the pkg-config files, since the dynamic library's dep on libsasl should be sufficient to satisfy the runtime linking. * Add FeatureSummary * Bump Earthly version, allow setting sasl options * Include dependencies for finding libsasl2 * Define more Earthly tasks * make _mongo_pick public, tweak signature of bool01 * Add Earthly target for testing against cxx-driver Build +test-cxx-driver to run a default mongo-cxx-driver using the local mongo-c-driver as the driver backend. * Better Earthly target docs * Fix SASL path finding for Windows, and use parallel Make
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first of several smaller changes that will go together into a full refactoring of the libmongoc build. This piece targets the SASL support library, with a few aside fixes.
Changed
The
FindSASL2.cmakemodule, despite having theFindname prefix, did not behave as a proper find-module. This refactoring changes the module to behave as a find-module and generates anIMPORTEDlibrary target against which libmongoc can link. This also resulted in changes to the installed mongoc targets to support the downstream users to link against a libsasl2. Whereas the prior install hard-coded the path to sasl2 within the -targets.cmake file, this change adds a level of indirection_mongoc::detail::saslthat is resolved at import-time to the SASL implementation that is available and that matches what the targets were built against.The code that decided which SASL to use was tangled, but the actual decision process is not complex. This refactors that process to a more straightforward implementation.
Additional Earthly targets and tasks have been defined to test the SASL on/off configurations.
Aside: Use the FeatureSummary module from CMake to print a definitive summary of enabled/disabled features and found/missing packages at the end of the configure process.
Aside: A
test-cxx-drivertarget in Earthly, which will perform a simple build of the C++ driver using the built result of the C driver. This does not have any EVG tasks, it's just for local testing (for now).