Skip to content

PHPC-2373 and PHPC-2374: Upgrade libmongoc 1.27.0 and libmongocrypt 1.10.0 #1540

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

Merged
merged 2 commits into from
May 7, 2024

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented May 1, 2024

@jmikola jmikola requested a review from alcaeus May 1, 2024 20:23
Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is missing changes to the versions in .evergreen/config/templates/build/build-libmongoc.yml.

Windows builds are also throwing errors, so something is wrong on that platform.

@jmikola
Copy link
Member Author

jmikola commented May 2, 2024

I'm discussing Windows build errors in #dbx-c-cxx:

Whilst upgrading to libmongoc 1.27.0, I'm hitting several unexpected build errors on Windows related to header includes. mcd-nsinfo.c and utlist.h in src/libmongoc/src both use <> instead of "" for includes, which is inconsistent from other source files.

I think the utlist.h problem can be resolved by declaring src/uthash as an include path (i.e. /I parameter) after src/libmongoc/src. It's currently declared first, as I do for src/common (see: config.w32). But I don't think there's a workaround for mcd-nsinfo.c, since it's having trouble picking up both mongoc-prelude.h and mcd-nsinfo.h, which have unique names.

Will follow up once I make some headway on that.

jmikola added 2 commits May 2, 2024 16:31
config.w32 requires an additional include path ("./src/libmongoc/src/libmongoc/src/mongoc") due to usage of angle bracket includes in mcd-nsinfo.c and utlist.h.
/I" + configure_module_dirname + "/src/libmongoc/src/libbson/src \
/I" + configure_module_dirname + "/src/libmongoc/src/libbson/src/jsonsl \
/I" + configure_module_dirname + "/src/libmongoc/src/libmongoc/src \
/I" + configure_module_dirname + "/src/libmongoc/src/libmongoc/src/mongoc \
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the suggested workaround for libmongoc using <> instead of "" for some new #include statements introduced in 1.27.0. CI suggests this is only necessary for Windows, but let me know if you think it'd be wise to duplicate this in config.m4. Alternatively, we can wait and see if any bug reports come in and address this in a patch release if needed (any failure would be quite obvious).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to do this for Windows only at this time 👍

@jmikola jmikola merged commit 49c520f into mongodb:master May 7, 2024
40 checks passed
@jmikola jmikola deleted the phpc-2373-2374 branch May 7, 2024 14:37
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