-
Notifications
You must be signed in to change notification settings - Fork 455
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
CDRIVER-5635 Exclude tests and examples from ALL target #1675
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comments.
The
if (NOT TARGET)
pattern is used to ensure compatibility with parent projects (where C Driver is imported viaadd_subdirectory()
) which may have already defined thetests
orexamples
target.
I would like to eventually consider prefixing target names (e.g. with mongoc
or similar) to avoid collisions. But I think that is outside of the scope of this PR.
@@ -97,30 +97,43 @@ fi | |||
|
|||
"${cmake_binary:?}" --version | |||
|
|||
if [[ "${OSTYPE}" == darwin* ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest removing if-block since script is Windows-specific.
declare compile_flags=( | ||
"/m" # Number of concurrent processes. No value=# of cpus | ||
) | ||
# MSBUild needs additional assistance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# MSBUild needs additional assistance. | |
# MSBuild needs additional assistance. |
Good call on prefixing target names. The CXX Driver may not be the only users using Renamed to use the |
Summary
Resolves CDRIVER-5635. Verified by this patch.
The ALL Target and Installation
Although tests and examples are correctly disabled via
ENABLE_TESTS
andENABLE_EXAMPLES
, they are not being correctly excluded from theALL
target when enabled. TheALL
target should only build targets which are required by theinstall
target (that is, targets which produce artifacts intended to be installed, with the exception of excluded components, which does not apply to our project. This is the same in spirit to themake && make install
pattern).Grepping the CMake build output for "Linking" is an approximate means of identifying the list of executable/library targets being built. The base commit currently emits the following list of artifacts for a simple
cmake --build <binary-dir>
(ALL target) configured with tests and examples enabled:This PR proposes reducing the list to only the following:
Note: man pages and html docs are not excluded, as they are also installed artifacts under
share/
.The new
tests
andexamples
custom targets are defined as a convenient means of compiling all test and example targets. Theif (NOT TARGET)
pattern is used to ensure compatibility with parent projects (where C Driver is imported viaadd_subdirectory()
) which may have already defined thetests
orexamples
target. The newtests
target is used by the updated EVG scripts to ensure test binaries are available for use by test tasks.Miscellaneous
CMAKE_BUILD_PARALLEL_LEVEL
instead of--parallel
,-- -j
, or/m
.UseMultiToolTask
andEnforceProcessCountAcrossBuilds
for better MSBuild parallelism with VS2019+.mongoc-cxx-check
from STATIC to OBJECT to avoid the unnecessary "Linking" step.EXAMPLES
variable (incorrect for its entire existence?).