Skip to content

CXX-1201 Support running examples against various topologies #1190

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 25 commits into from
Aug 15, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Aug 13, 2024

Summary

Resolves CXX-1201. Verified by this patch.

The changes in this PR are a precursor to the addition of API doc examples in CXX-3082.

Description

The goal of this PR is to prepare for the addition of the examples/api subdirectory. This directory will be dedicated to example code that is directly imported into API docs via the \snippet special command (which permits greater control over the imported contents than the blunt \include command). The existing examples/bsoncxx and examples/mongocxx executables are preserved for backward compatibility and as a set of standalone example programs not related to API docs.

Cleanup

The CMakeLists.txt files under examples subdirectories were removed in favor of file(GLOB_RECURSE), similar to what is already being done for public header files. This will allow for flexible addition, removal, and relocation of example code (although it is expected to mirror the include directory structure). The add_subdirectory and projects subdirectories are not actually used by the primary CMake configuration (used via test scripts instead), so their CMake config files have been removed and their contents manually listed in the distribution file list.

examples vs. run-examples

The build vs. run targets were redesigned to completely separate the "build the examples" commands from the "run the examples" commands. This reduces redundant build tasks (especially with MSVC) and also permits parallelism during building of executables. However, this requires the examples target to be manually built prior to the run-examples target, otherwise the run-examples target will fail attempting to execute non-existent executables. EVG scripts were updated accordingly. This change also required the addition of --parallel 1 (overriding CMAKE_BUILD_PARALLEL_LEVEL) to ensure example executables are executed sequentually rather than in parallel (they are not designed to run in parallel).

MONGOCXX_TEST_TOPOLOGY

Several examples executables under examples/mongocxx were being skipped entirely (both execution and compilation) due to depending on particular server topologies to run correctly (CXX-1201). Following the changes in this PR, all examples are now built by CI. However, some examples are still not run. Specifically, any executables under examples/mongocxx containing "encryption" in their name are skipped via the SKIP_RUN_EXAMPLES option in the new add_examples_executable() CMake function. Enabling these examples to be run is deferred to a separate PR (e.g. by relying on pre-installed C Driver spawning mongocryptd + signalling with a MONGOCXX_TEST_WITH_LIBMONGOCRYPT env var?).

This PR introduces the MONGOCXX_TEST_TOPOLOGY environment variable which serves two purposes: 1) indicate the example executables are being executed within our CI environment (users are not expected to set this) and 2) indicate the server topology currently present in our CI environment (as defined by the preceeding "start_mongod" EVG function per task definition). This variable is set by each call to the "test" EVG function (which invokes test.sh).

Not all example executables require checking the MONGOCXX_TEST_TOPOLOGY variable. e.g. the examples/mongocxx/connect executable is defined to to be directly invoked by the "test auth" EVG function (via .evergreen/connect.sh). To avoid complicating the EVG config, only invocations of the "test" EVG function defines MONGOCXX_TEST_TOPOLOGY. We may want to eventually separate running example executables from the test.sh script.

Updates to Examples and Miscellaneous

The rest of this PR contains updates to example code to permit their execution in CI without causing issues, as well as other miscellaneous improvements. These include:

  • checking for MONGOCXX_TEST_TOPOLOGY and its value as needed.
  • changing the value of the replicaSet URI option to match mongo orchestration configs (replset -> repl0).
  • disabling printing of error/exception messages to avoid triggering MSBuild error message detection (which is treated as build failure). 💢
  • printing example output to stdout instead of stderr to avoid interleaving in EVG output.
  • consistent use of --config <build_type> to specify the build type instead of MSBuild-specific flags.

@eramongodb eramongodb requested a review from kevinAlbs August 13, 2024 21:22
@eramongodb eramongodb self-assigned this Aug 13, 2024
@kevinAlbs kevinAlbs requested a review from rcsanchez97 August 14, 2024 16:48
@eramongodb
Copy link
Contributor Author

eramongodb commented Aug 15, 2024

Resolved merge conflicts and cherry-picked some additional changes:

  • Added the file(GLOB_RECURSE) command for API doc examples (no examples committed yet).
  • Removed compilation of fwd.hpp headers to avoid rebuild of libraries on doc comment updates. Given the fwd.hpp headers are very simple in content (#pragma once + #include directives), standalone include checks are deferred to the included headers' components instead.
  • Added support for Markdown files under docs/api which will be used for Topic pages which will contain "How To" examples for API docs.
  • Split run-examples by subdirectory (api, bsoncxx, mongocxx) to permit individual execution of example groups (in particular to avoid the noise of bsoncxx and mongocxx examples output).

Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

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

LGTM, with one small recommended change.

if (${EXAMPLE} MATCHES "encryption")
continue()
if (${source} MATCHES "encryption")
set(skip_run_examples ON)
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the name skip_run_examples quite confusing. On the first read-through I was convinced that anytime a matching example was encountered that it would skip running all examples. I recognize that this variable is distinct from SKIP_RUN_EXAMPLES (since CMake is case sensitive when it comes to variable names) and I did eventually figure out that the former is used to set the value of the latter as a property for the individual example executable. However, I recommend renaming this variable to skip_run_this_example.

Copy link
Contributor Author

@eramongodb eramongodb Aug 15, 2024

Choose a reason for hiding this comment

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

Good point. There is some double-negation occurring with the current setup relative to the default:

  • Is this example added to run-examples?
    • Yes (by default), unless:
      • No Skip -> Yes (Yes -> No -> Yes) (<--)
      • Yes Skip -> No (Yes -> Yes -> No)

Renamed SKIP_RUN_EXAMPLES -> ADD_TO_RUN_EXAMPLES and skip_run_examples -> add_to_run_examples so there is only one level of negation relative to the default + no singular vs. plural confusion.

  • Is this example added to run-examples?
    • Yes (by default), unless:
      • No Add -> No (Yes -> No -> No)
      • Yes Add -> Yes (Yes -> Yes -> Yes)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. That is much more clear.

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.

LGTM with suggested comment.

@@ -31,6 +31,9 @@ using bsoncxx::builder::basic::kvp;
using bsoncxx::builder::basic::make_document;

int main() {
// No known way to set MSBuild IgnoreStandardErrorWarningFormat=true via CMake or CLI.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// No known way to set MSBuild IgnoreStandardErrorWarningFormat=true via CMake or CLI.
// Do not print error messages when run in CI. Avoids MSBuild error message detection causing build failures.
// No known way to set MSBuild IgnoreStandardErrorWarningFormat=true via CMake or CLI.

@eramongodb eramongodb merged commit 7918494 into mongodb:master Aug 15, 2024
1 check was pending
@eramongodb eramongodb deleted the cxx-1201 branch August 15, 2024 18:18
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