Skip to content

cleanup: shorten Bazel project name#10207

Merged
coryan merged 9 commits intogoogleapis:mainfrom
coryan:cleanup-shorten-workspace-name
Nov 8, 2022
Merged

cleanup: shorten Bazel project name#10207
coryan merged 9 commits intogoogleapis:mainfrom
coryan:cleanup-shorten-workspace-name

Conversation

@coryan
Copy link
Copy Markdown
Contributor

@coryan coryan commented Nov 8, 2022

Using a shorter project name will avoid problems on Windows, where Bazel
can create paths exceeding the MSVC hardcoded limits (260 characters).
Note that, AFAICT, this is a bug in the compiler. Windows supports
longer paths, but the compiler cannot use them for some reason. Bazel
does not help either, by creating deeply nested names for the object
files.

Fixes #9340


This change is Reviewable

Comment thread WORKSPACE.bazel
# limitations under the License.

workspace(name = "com_github_googleapis_google_cloud_cpp")
workspace(name = "google_cloud_cpp")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is a breaking change.

Customers that depend on @com_github_googleapis_google_cloud_cpp//... have to update their BUILD files.

is there some alias we can leave?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see that the name field in http_archive() is required. So maybe this is not a problem.

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.

I do not think this is breaking. Customers can continue to use something like this:

http_archive(
    name = "com_github_googleapis_google_cloud_cpp",
    sha256 = "245e198e29c4ec19734cc99ef631daaefbdb874307fc3743e22514ee8bcb36c4",
    strip_prefix = "google-cloud-cpp-2.4.0",
    url = "https://github.com/googleapis/google-cloud-cpp/archive/v2.4.0.tar.gz",
)

And use the @com_github_googleapis_google_cloud_cpp//... in their build scripts (or Bazel files). It used to be impossible to use any other name because we hard-coded @com_github_googleapis_google_cloud_cpp in our own BUILD files, but I have managed to remove that restriction (using Label() when possible, changing some stuff to be "patches" instead of references).

No, we cannot leave an alias. There is a long discussion in bazelbuild/bazel#3219 explaining what the Bazel team thinks is a solution, and everybody else telling them it is not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please check my logic here. I could still be misunderstanding....

Considering this example:

# Load Abseil
if "com_google_absl" not in native.existing_rules():
http_archive(
name = "com_google_absl",
strip_prefix = "abseil-cpp-20220623.1",
urls = [
"https://github.com/abseil/abseil-cpp/archive/20220623.1.tar.gz",
],
sha256 = "91ac87d30cc6d79f9ab974c51874a704de9c2647c40f6932597329a282217ba8",
)

Let's say abseil changes their project name to com_google_abseil_new. All of our code keeps working. No problem.

But if we change our load_deps() to check for "com_google_abseil_new", it would be a breaking change. (e.g. if a downstream app is pinning abseil to a specific version, we would ignore it in favor of our default version of abseil.).

So are we to blame for the break? or is it really abseil....

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.

But if we change our load_deps() to check for "com_google_abseil_new"

Why would we do that? We are trying to find out if something else has loaded Abseil with the name we intend to use. The name that Abseil calls itself is not material here.

A different question would be:

  • What if a downstream application starts loading Abseil with the com_google_abseil_new name?

They could do that. And that may break things if they loaded a different version (ODR and diamond dependencies get messy quickly). In the Bazel world it is the job of the leaf WORKSPACE to satisfy all the dependencies consistently. That is cumbersome, and thus the invention of bzlmod.

For example, if gRPC starts using a different name for Abseil then we need to use the same name, or introduce some kind of repository mapping (and/or bind()) to make sure gRPC uses the same version we loaded.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the Bazel world it is the job of the leaf WORKSPACE to satisfy all the dependencies consistently.

Ah. I did not realize this was the paradigm. That clears things up. Thanks.

@coryan coryan force-pushed the cleanup-shorten-workspace-name branch from 0db7fd9 to 79fca3d Compare November 8, 2022 15:24
@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: 0db7fd98d229b851f038af920132a4cc0a751571

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 8, 2022

Codecov Report

Base: 93.98% // Head: 93.98% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (40fa05e) compared to base (22ecaf5).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10207      +/-   ##
==========================================
- Coverage   93.98%   93.98%   -0.01%     
==========================================
  Files        1531     1531              
  Lines      141082   141082              
==========================================
- Hits       132597   132596       -1     
- Misses       8485     8486       +1     
Impacted Files Coverage Δ
generator/internal/scaffold_generator.cc 85.13% <ø> (ø)
...e/cloud/spanner/testing/cleanup_stale_instances.cc 68.85% <0.00%> (-6.56%) ⬇️
...e/cloud/pubsublite/internal/alarm_registry_impl.cc 97.05% <0.00%> (-2.95%) ⬇️
google/cloud/completion_queue_test.cc 97.13% <0.00%> (-0.20%) ⬇️
...cloud/pubsub/internal/subscription_session_test.cc 98.49% <0.00%> (-0.17%) ⬇️
google/cloud/pubsub/samples/samples.cc 90.70% <0.00%> (-0.08%) ⬇️
...ud/spanner/integration_tests/client_stress_test.cc 84.86% <0.00%> (+0.65%) ⬆️
google/cloud/bigtable/async_read_stream_test.cc 97.99% <0.00%> (+0.66%) ⬆️
...integration_tests/schema_admin_integration_test.cc 100.00% <0.00%> (+1.11%) ⬆️
google/cloud/internal/async_connection_ready.cc 93.61% <0.00%> (+4.25%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

Google Cloud Build Logs
For commit: 79fca3dc59b9b6871b8232d3b3e78384c5ab5877

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

Using a shorter project name will avoid problems on Windows, where Bazel
can create paths exceeding the MSVC hardcoded limits (260 characters).
Note that, AFAICT, this is a bug in the compiler. Windows supports
longer paths, but the compiler cannot use them for some reason. Bazel
does not help either, by creating deeply nested names for the object
files.
@coryan coryan force-pushed the cleanup-shorten-workspace-name branch from 79fca3d to 3388b81 Compare November 8, 2022 16:39
@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: 3388b81aa46cb4b6c33ed7a2d85edf1c188a800a

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

@coryan coryan marked this pull request as ready for review November 8, 2022 17:28
@coryan coryan requested a review from a team November 8, 2022 17:28
@google-cloud-cpp-bot
Copy link
Copy Markdown
Contributor

Google Cloud Build Logs
For commit: 0c96e783524ddef05a6206c968eb91713a4c9725

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

Copy link
Copy Markdown
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

FYI: I think this fixes the issue linked in the PR description

@coryan
Copy link
Copy Markdown
Contributor Author

coryan commented Nov 8, 2022

FYI: I think this fixes the issue linked in the PR description

I was missing one change in CI, fixed, and will update the description.

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

Google Cloud Build Logs
For commit: 40fa05e8fbb86a44404de95cd6b7735a7caa6f15

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

@coryan coryan merged commit f617e71 into googleapis:main Nov 8, 2022
@coryan coryan deleted the cleanup-shorten-workspace-name branch November 8, 2022 22:02
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.

Generate library for beyondcorp.googleapis.com

3 participants