Skip to content
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

Add MODULE.bazel to support bzlmod #4368

Closed
wants to merge 1 commit into from
Closed

Add MODULE.bazel to support bzlmod #4368

wants to merge 1 commit into from

Conversation

Vertexwahn
Copy link
Contributor

@Vertexwahn Vertexwahn commented Sep 5, 2023

Life with Bzlmod is easier if every dependency provides a MODULE.bazel file. Therefore, the main purpose of this PR is to introduce a MODULE.bazel file in googletest. The MODULE.bazel can coexist with the WORKSPACE file. Users have to enable bzlmod via --enable_bzlmod flag (more details to the Bzlmod migration plan can be found here).

This PR is similar to #4118 (review)

Differences:

  • Uses newer versions of modules
  • Uses archive_ovrride to support live at head philosophy
  • Update checkout action (v3 -> v4) in GitHub workflow
  • Reformates old WORKSPACE file using buildifier

Further remarks:

  • module_dot_bazel.patch is needed since abseil-cpp does not provide a MODULE.bazel file unit now - this is a chicken egg situation between the public GitHub repos of googletest and abseil

I recommend closing #4118 (review) and merging this PR

@Vertexwahn
Copy link
Contributor Author

@derekmauro Please review!

Copy link
Member

@derekmauro derekmauro left a comment

Choose a reason for hiding this comment

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

This seems to almost work. I think where this approach fails is when we try to do a release, because of the chicken-egg problem with Abseil.

The GoogleTest and Abseil releases are coordinated. When we release GoogleTest, we bump all versions to the latest in the WORKSPACE file, and do the release, like normal. Then we do the Abseil release, using changing the WORKSPACE file to use the GoogleTest release. If you want to use them both, you just create a WORKSPACE file with both released versions, and they will work together.

The chicken-egg problem only happens with the MODULE.bazel file. Since the versions must be named before they can be referred to, only one of GoogleTest and Abseil can point to the latest released version. Unless I've missed something, any user of both would have to override at least one of them.

It almost seems that the best solution is to keep the patch in the Bazel Central Repository where the versions can be updated simultaneously after being published. We can add this step to our release process and officially manage it, if there is no other solution.

/cc @meteorcloudy

@meteorcloudy
Copy link

Can we also upstream the MODULE.bazel to abseil-cpp? Then you won't need to add a patch for the archive_override.

As long as both googletest and abseil-cpp are published to the BCR, the end users can always specify the latest version for both of them, and Bzlmod will resolve the dependencies. I don't see any chicken-egg problem?

@Vertexwahn
Copy link
Contributor Author

@derekmauro Should I open a PR on abseil-cpp?

@derekmauro
Copy link
Member

As long as both googletest and abseil-cpp are published to the BCR, the end users can always specify the latest version for both of them, and Bzlmod will resolve the dependencies. I don't see any chicken-egg problem?

Sure, that works. But what if you are only using GoogleTest directly? You might not even know that you depend on Abseil.

@derekmauro
Copy link
Member

@derekmauro Should I open a PR on abseil-cpp?

Let's figure out how to resolve this first.

@meteorcloudy
Copy link

Sure, that works. But what if you are only using GoogleTest directly? You might not even know that you depend on Abseil.

I assume googletest will introduce a suitable abseil version automatically? It might not be the latest one, but it should work with googletest?

@derekmauro
Copy link
Member

I assume googletest will introduce a suitable abseil version automatically? It might not be the latest one, but it should work with googletest?

What does it mean to "introduce a suitable abseil version automatically"?

@meteorcloudy
Copy link

In googletest's MODULE.bazel file, depend on abseil-cpp via a bazel_dep statement.

@derekmauro
Copy link
Member

I think this misses the point I made above. Let me try an explicit example.

Abseil and GoogleTest are both at version 1.
Abseil introduces feature X in a dev commit.
GoogleTest uses feature X in a dev commit. It is able to do this because it uses the archive_override feature to support live at head philosophy.
GoogleTest introduces feature Y in a dev commit.
Abseil uses feature Y in a dev commit. Again, it is able to this because it uses the archive_override feature to support live at head philosophy.

Now it comes time to release Abseil and GoogleTest version 2.
If I release GoogleTest version 2 first, I can't refer to any released version of Abseil in MODULE.bazel, because none of them have feature X.
If I release Abseil version 2 first, I can't refer to any released version of GoogleTest in MODULE.bazel, because none of them have feature Y.

@meteorcloudy
Copy link

meteorcloudy commented Sep 13, 2023

Thanks for the example, I see the problem now.

I guess, we need to decouple the release processes and increase the frequency to address this issue.

Imagine if you release a new version every time you had a new commit to each of the libraries, we won't have this problem since this will fallback to the WORKSPACE use case (where you can point to any commit).

But that's the extreme case, we only need to have a new abseil version once feature X is introduced so that googletests can depend on a released version of abseil and vice versa.

Do you think this would be feasible? I'm not sure how expensive it is to make a new release for those two projects.

copybara-service bot pushed a commit to abseil/abseil-cpp that referenced this pull request Jan 10, 2024
…to unblock

releasing Abseil and GoogleTest.

GoogleTest referenced this internal file and this internal trait.  Since
simultaneous releases are not possible since once release must reference
another, we will temporarily add this back.
https://github.com/google/googletest/blob/v1.14.x/googletest/include/gtest/gtest-printers.h#L119

google/googletest#4368 (comment)
google/googletest#4368 (comment)

PiperOrigin-RevId: 597073935
Change-Id: I7c2697a212dc477fd25770777445c64cfee73745
@derekmauro
Copy link
Member

Support has been added in 6a59382. We will publish support to BCR after the next release.

@derekmauro derekmauro closed this Jan 25, 2024
netkex pushed a commit to netkex/abseil-cpp that referenced this pull request Apr 3, 2024
…to unblock

releasing Abseil and GoogleTest.

GoogleTest referenced this internal file and this internal trait.  Since
simultaneous releases are not possible since once release must reference
another, we will temporarily add this back.
https://github.com/google/googletest/blob/v1.14.x/googletest/include/gtest/gtest-printers.h#L119

google/googletest#4368 (comment)
google/googletest#4368 (comment)

PiperOrigin-RevId: 597073935
Change-Id: I7c2697a212dc477fd25770777445c64cfee73745
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.

None yet

3 participants