Skip to content

Conversation

@ahsonkhan
Copy link
Contributor

Fixes #5334

this is internal only and reduce chances of collision.
Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

Please remove any mentions of BUILD_TESTING cmake flag in our docs and cmake options. I've found two:

  1. <td>BUILD_TESTING</td>
    <td>Generates Unit Test for compilation. CMake will automatically download and build g-test.<br>After Compiling, use `ctest` to run Unit Test.</td>
    <td>OFF</td>
  2. option(BUILD_TESTING "Build test cases" OFF)

    There probably should be a paragraph saying that when the project is build with unit tests, those SDK binaries that you get as a result of such build, should not be used by customer's code. They are for unit tests only.

@ahsonkhan
Copy link
Contributor Author

ahsonkhan commented Feb 29, 2024

Please remove any mentions of BUILD_TESTING cmake flag in our docs and cmake options.

BUILD_TESTING is still a valid cmake flag that is intended for use by contributors to the repo. It is also used in our CI pipelines for legs that build tests and ones that only build the source. It isn't meant for end users of the SDK, but is certainly needed by and intended for folks who build, test, and contribute to this OSS project.

I would not want to remove it from the contribution guide because then no new contribute or member of the project would know where to go to figure out steps to build the tests. The contributing guide is for that exact purpose.

Instead, as part of #5335, I will add a note to clarify it isn't meant to be used in production when consuming the SDK, and only meant for contributors to the repo.

There probably should be a paragraph saying that when the project is build with unit tests, those SDK binaries that you get as a result of such build, should not be used by customer's code. They are for unit tests only.

Agreed.

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

Thanks, I think that'd work too.

@ahsonkhan ahsonkhan enabled auto-merge (squash) March 1, 2024 05:02
@ahsonkhan ahsonkhan merged commit 256c2df into Azure:main Mar 1, 2024
@ahsonkhan ahsonkhan deleted the UpdateCompilerDef branch March 1, 2024 06:21
antkmsft added a commit to antkmsft/azure-sdk-for-cpp that referenced this pull request Mar 7, 2024
antkmsft added a commit that referenced this pull request Mar 7, 2024
…R_TESTS` (#5416)

* Revert "Remove the use of ifdef for TESTING_BUILD in KeyVault clients, where (#5406)"

This reverts commit 2d8c940.

* Revert "Rename the TESTING_BUILD macro to be _azure_TESTING_BUILD to highlight (#5390)"

This reverts commit 256c2df.

* Revert "Use new macros in existing surface area, so that classes marked as final don't have virtual methods. (#5389)"

This reverts commit 3d7eadd.

* Revert "For new surface area, classes marked as final should not have virtual methods. (#5331)"

This reverts commit ddd0f4b.

---------

Co-authored-by: Anton Kolesnyk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Change the TESTING_BUILD macro to make it clear it doesn't collide with a customer macro.

2 participants