Skip to content

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
@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