Skip to content

CDRIVER-4249 Apply _DEFAULT_SOURCE to all subdirectories #920

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 9 commits into from
Jan 5, 2022

Conversation

eramongodb
Copy link
Contributor

Per CDRIVER-4249, this PR cleans up feature test macros used to enable non-conforming C language extensions such as POSIX and BSD library functions.

The _BSD_SOURCE macro is a deprecated alias of _DEFAULT_SOURCE, so it was removed in favor of the already-present _DEFAULT_SOURCE.

There was a choice available as to whether to use _DEFAULT_SOURCE or _GNU_SOURCE. The difference being (according to GNU documentation):

  • _GNU_SOURCE: everything is included: ISO C89, ISO C99, POSIX.1, POSIX.2, BSD, SVID, X/Open, LFS, and GNU extensions. In the cases where POSIX.1 conflicts with BSD, the POSIX definitions take precedence.
  • _DEFAULT_SOURCE: most features are included apart from X/Open, LFS and GNU extensions: the effect is to enable features from the 2008 edition of POSIX, as well as certain BSD and SVID features without a separate feature test macro to control them.

The documentation recommends _GNU_SOURCE for new programs. However, _GNU_SOURCE preferring POSIX definitions over BSD seems to cause strerror_r() compatibility/detection issues on Darwin that are not present with _DEFAULT_SOURCE. Because _DEFAULT_SOURCE does not prompt these issues while still enabling the set of features required and used by the C driver, _DEFAULT_SOURCE was selected and _GNU_SOURCE removed.

To permit validating changes on Darwin, this PR includes a drive-by fix of build failures on Darwin due to #890. Since this fix requires modifying the zlib bundle (last updated in 2018), I would appreciate some confirmation as to whether the changes made are appropriate. If we wish to leave the zlib sources unmodified, we can choose to selectively exclude _DEFAULT_SOURCE during compilation of the zlib bundle instead.

@eramongodb eramongodb self-assigned this Dec 23, 2021
@eramongodb
Copy link
Contributor Author

@jmikola Regarding PHPC, the changes in this PR suggest that defining _DEFAULT_SOURCE alone should (hopefully) suffice for building the entire C driver via autotools. Please let me know if you observe otherwise.

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

Regarding PHPC, the changes in this PR suggest that defining _DEFAULT_SOURCE alone should (hopefully) suffice for building the entire C driver via autotools. Please let me know if you observe otherwise.

I can confirm that replacing -D_XOPEN_SOURCE=700 with -D_DEFAULT_SOURCE in our bundled CFLAGS allows libmongoc to once again compile without error. I'll defer to Kevin on the rest of this PR, but this LGTM.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Thank you for the drive-by fix for Darwin tasks with zlib! I request not modifying the zlib source directly, but otherwise LGTM.

@@ -27,6 +27,7 @@ check_include_file(stddef.h HAVE_STDDEF_H)
#
# Check to see if we have large file support
#
cmake_push_check_state()
Copy link
Collaborator

Choose a reason for hiding this comment

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

To permit validating changes on Darwin, this PR includes a drive-by fix of build failures on Darwin due to #890. Since this fix requires modifying the zlib bundle (last updated in 2018), I would appreciate some confirmation as to whether the changes made are appropriate. If we wish to leave the zlib sources unmodified, we can choose to selectively exclude _DEFAULT_SOURCE during compilation of the zlib bundle instead.

I prefer leaving the zlib sources unmodified. It more easily enables updating the bundled zlib. E.g. if zlib has a priority patch release (e.g. to fix a security bug) it may simplify updating the bundled zlib.

@eramongodb eramongodb requested a review from kevinAlbs January 4, 2022 18:31
endif ()
if (HAVE_STDARG_H)
add_definitions (-DHAVE_STDARG_H)
endif ()
Copy link
Member

Choose a reason for hiding this comment

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

Offhand, PHP was already defining HAVE_UNISTD_H for bundled zlib. It was trivial to make the change for HAVE_STDARG_H, although I didn't notice any build failure omitting it on Ubuntu 20.04.

LGTM!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Lack of) <stdarg.h> was not causing issues, as far as I could observe. HAVE_STDARG_H was just added to compliment the addition of HAVE_UNISTD_H for consistency. The (lack of) <unistd.h> was the relevant issue being addressed.

@eramongodb eramongodb merged commit 9d2d8b1 into mongodb:master Jan 5, 2022
@eramongodb eramongodb deleted the cdriver-4249 branch January 5, 2022 15:19
jmikola referenced this pull request in mongodb/mongo-php-driver Jan 5, 2022
This allows PHPC to defer entirely to libmongoc for cross-option URI validation

Bump libmongoc to 1.21-dev

Build changes are ported from upstream CMake changes (see: CDRIVER-4249)
jmikola added a commit to mongodb/mongo-php-driver that referenced this pull request Jan 5, 2022
Reverts a change introduced in ab44b0c. This was previously in mongodb/mongo-c-driver#920 but removed before merging in mongodb/mongo-c-driver@9d2d8b1.
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.

3 participants