Skip to content

refactor(common): versioned namespace includes major, minor, patch#7464

Merged
devjgm merged 3 commits intogoogleapis:mainfrom
devjgm:versioned-inline-ns
Oct 14, 2021
Merged

refactor(common): versioned namespace includes major, minor, patch#7464
devjgm merged 3 commits intogoogleapis:mainfrom
devjgm:versioned-inline-ns

Conversation

@devjgm
Copy link
Copy Markdown
Contributor

@devjgm devjgm commented Oct 14, 2021

Fixes: #5976

This PR changes our inline namespace to include the major, minor, and
patch numbers. It's been discussed that we should not include the patch
number, and that's OK with me if there's a benefit to doing so.

This PR also adds backward compatibility namespace aliases for both
v1 and gcpcxxV1 to avoid breaking users. This does introduce the
gcpcxxV1 alias in non-generated code where it didn't previously exist
(and the same with v1 in generated code), but this solution seems to
work fine and seems significantly simpler than any alternative that I
was able to think of.

There's no place to use @deprecated on these symbols, and indeed they
don't even show up in Doxygen documentation. The only indication of
deprecation is a mention in the CHANGELOG.md file that's included in
this PR.


This change is Reviewable

Fixes: googleapis#5976

This PR changes our inline namespace to include the major, minor, and
patch numbers. It's been discussed that we should not include the patch
number, and that's OK with me if there's a benefit to doing so.

This PR also adds backward compatibility namespace aliases for both
`v1` and `gcpcxxV1` to avoid breaking users. This does introduce the
`gcpcxxV1` alias in non-generated code where it didn't previously exist
(and the same with `v1` in generated code), but this solution seems to
work fine and seems significantly simpler than any alternative that I
was able to think of.

There's no place to use `@deprecated` on these symbols, and indeed they
don't even show up in Doxygen documentation. The only indication of
deprecation is a mention in the `CHANGELOG.md` file that's included in
this PR.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2021
@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: 280a50e338887b90b8f5a3a0379567ba65f261e0

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 14, 2021

Codecov Report

Merging #7464 (ecf59c5) into main (c0cb1c5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7464   +/-   ##
=======================================
  Coverage   93.64%   93.64%           
=======================================
  Files        1364     1364           
  Lines      118362   118362           
=======================================
  Hits       110842   110842           
  Misses       7520     7520           
Impacted Files Coverage Δ
google/cloud/version.h 100.00% <ø> (ø)
...bigtable/examples/bigtable_hello_instance_admin.cc 81.31% <0.00%> (-2.20%) ⬇️
google/cloud/pubsub/subscriber_connection_test.cc 97.18% <0.00%> (-0.71%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 98.00% <0.00%> (ø)
...le/cloud/storage/internal/curl_download_request.cc 89.62% <0.00%> (+0.37%) ⬆️
google/cloud/grpc_error_delegate.cc 100.00% <0.00%> (+4.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0cb1c5...ecf59c5. Read the comment docs.

@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: ecf59c5cc6a6d58086060c8e3a37f076285e0ff2

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@devjgm devjgm marked this pull request as ready for review October 14, 2021 13:52
@devjgm devjgm requested a review from a team October 14, 2021 13:52
Comment thread release/README.md
you can `CTRL-C` the build after CMake's configure step is finished.
`ci/cloudbuild/build.sh -t clang-tidy-pr` will do this, and you can `CTRL-C`
the build after CMake's configure step is finished.
- Update the ABI baseline to include the new version numbers in the inline
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting. I had thought about this "enforcing" an ABI break on each release, but I had not thought about the impact on our baselines. I am not sure if this is good, bad, or indifferent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as our ABI baselines dumps are concerned, I think this change is roughly equivalent to before, because we were already updating the baseline with each release, but now we need to do it at a different point in the release process.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, I think we have a problem for patch releases:

  • We create release 1.100.0.
  • We update the version numbers.
  • We update the baseline, so all its symbols contain v1_101_0, e.g. assume google::cloud::v1_101_0::Bar exists.
  • Commit the baseline to main.
  • Some time goes by we add symbol google::cloud::v1_101_0::Foo, we don't necessarily update the baseline because adding symbols does not break the CI builds.
  • We create the v1.101.0 release.
  • The v1.101.x branch contains the baseline with v1_101_0 from whatever point it was last updated (which may not be the release point).
  • Let's say we want to create v1.101.1
  • We create a PR ... and send it to the CI builds.
  • The CI builds will detect some changes, but will not detect other changes:
    • Recall that the baseline was not updated when v1.101.0 was created, it is "stale"
    • Removing google::cloud::v1_101_0::Bar would be detected.
    • Removing google::cloud::v1_101_0::Foo would not be detected.

@devjgm devjgm merged commit 0d5e25e into googleapis:main Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider changing our inline namespace name with each release

4 participants