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

refactor: add MODULE.bazel file for bzlmod #16013

Closed
wants to merge 1 commit into from
Closed

Conversation

alexeagle
Copy link
Collaborator

Step towards #14564. The strategy is:

  1. Burndown the set of patches which are currently applied only when protobuf is published to BCR, by applying those patches to the repo.
  2. In cases where a patch can't be accepted to the repo, it can be in the .bcr folder.
  3. Finish feat: automate publishing releases to Bazel Central Registry #14565 by configuring the Publish-to-BCR github app.

In this PR, we remove the first patch from the stack of the most recent release: https://github.com/bazelbuild/bazel-central-registry/tree/main/modules/protobuf/23.1/patches

If the maintainers prefer, I could apply a number of these patches in one PR.

Step towards #14564. The strategy is:

1. Burndown the set of patches which are currently applied only when protobuf is published to BCR, by applying those patches to the repo.
2. In cases where a patch can't be accepted to the repo, it can be in the .bcr folder.
3. Finish #14565 by configuring the Publish-to-BCR github app.
@alexeagle alexeagle requested a review from a team as a code owner March 1, 2024 01:18
@alexeagle alexeagle requested review from haberman and removed request for a team March 1, 2024 01:18
alexeagle added a commit to alexeagle/protobuf that referenced this pull request Mar 1, 2024
Note, recent releases on the BCR have a patch set applied, and it seems these patches are developed
independently to "fix" each protobuf release, rather than make changes to protobuf repo.

The effect of this PR will be to create a *broken* publish to BCR for each protobuf release.
At least this red PR on BCR will be our indication that the patches need to be manually replayed there.

In parallel, starting with protocolbuffers#16013 I'll apply as many of those patches to the protobuf repo as possible.
That will reduce the manual effort for each release.
alexeagle added a commit to alexeagle/protobuf that referenced this pull request Mar 1, 2024
These files copied from the last release:
https://github.com/bazelbuild/bazel-central-registry/tree/main/modules/protobuf/23.1

Note, recent releases on the BCR have a patch set applied, and it seems these patches are developed
independently to "fix" each protobuf release, rather than make changes to protobuf repo.

The effect of this PR will be to create a *broken* publish to BCR for each protobuf release.
At least this red PR on BCR will be our indication that the patches need to be manually replayed there.

In parallel, starting with protocolbuffers#16013 I'll apply as many of those patches to the protobuf repo as possible.
That will reduce the manual effort for each release.
@mmorel-35
Copy link

I also started this #15976 earlier ;)
I believe you intend to make it one patch at a time which is going to be easier to review.

@alexeagle
Copy link
Collaborator Author

Yes and also to test the waters to see what the maintainers plan to do

MODULE.bazel Show resolved Hide resolved
MODULE.bazel Show resolved Hide resolved
MODULE.bazel Show resolved Hide resolved
@alexeagle
Copy link
Collaborator Author

Hey @haberman could we get some feedback from maintainers?

@mkruskal-google mkruskal-google requested review from mkruskal-google and zhangskz and removed request for haberman March 7, 2024 19:28
@mkruskal-google
Copy link
Member

mkruskal-google commented Mar 7, 2024

Hey Alex, Sandy's OOO until next week, but I can try digging into this in the meantime.

QQ: does any of this take effect immediately or does it only go public after our next release?

@alexeagle
Copy link
Collaborator Author

Oh, you're going to have a lot of questions since Bazel is so different from Blaze, and you have no repository rules or module extensions or MODULE.bazel files...
Anyhow this only would have an effect if a Bazel user with --enable_bzlmod used a git SHA to fetch protobuf rather than getting a release, like with git_override. That wouldn't work for them anyhow, so the simple answer is no, it doesn't have an effect until we release and also publish that release to the BCR

bazel_dep(name = "rules_cc", version = "0.0.1")
bazel_dep(name = "rules_proto", version = "4.0.0")
bazel_dep(name = "rules_java", version = "4.0.0")
bazel_dep(name = "rules_pkg", version = "0.7.0")
Copy link
Member

Choose a reason for hiding this comment

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

All of these dependencies seem older than what we're using now. Why are we downgrading?

module(
name = "protobuf",
compatibility_level = 1,
version = "23.1",
Copy link
Member

Choose a reason for hiding this comment

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

We're currently working on 26.0 (you could say main is on 27.0), it seems like all of these dependencies are stale

@mkruskal-google
Copy link
Member

IIUC, this is a direct port from the patches of 23.1 from BCR. While adding these changes incrementally makes sense, I don't see the value in submitting broken configs that we can't even test. Ideally we would have bzlmod CI setup and make these changes correctly from the start

@alexeagle
Copy link
Collaborator Author

You'd need to add a new test fixture in this repo to verify the next release will work in bzlmod. The BCR presubmit test only runs after the release is cut and the artifact is published.

copybara-service bot pushed a commit that referenced this pull request Mar 12, 2024
Note, recent releases on the BCR have a patch set applied, and it seems these patches are developed independently to "fix" each protobuf release, rather than make changes to protobuf repo.

The effect of this PR will be to create a *broken* publish to BCR for each protobuf release. At least this red PR on BCR will be our indication that the patches need to be manually replayed there.

In parallel, starting with #16013 I'll apply as many of those patches to the protobuf repo as possible. That will reduce the manual effort for each release.

Replaces #14565 which originated from my fork so the tests wouldn't run.

Closes #16014

COPYBARA_INTEGRATE_REVIEW=#16014 from protocolbuffers:bcr e17d9c8
PiperOrigin-RevId: 615026796
@mkruskal-google
Copy link
Member

Yea I'd like to add new tests either with this PR or right after, but my point is more that this config is incorrect and won't work IIUC. It's based on a very old version, so I don't see the value in submitting this as-is

@alexeagle
Copy link
Collaborator Author

I don't see the value in submitting this as-is

Okay, I'll close this then. I'll reach out to you on chat so we can figure out what shape you want these new tests to be.

@alexeagle alexeagle closed this Mar 26, 2024
@alexeagle
Copy link
Collaborator Author

Replaced by #16319

deannagarcia pushed a commit to deannagarcia/protobuf that referenced this pull request Jun 20, 2024
…lbuffers#16014)

Note, recent releases on the BCR have a patch set applied, and it seems these patches are developed independently to "fix" each protobuf release, rather than make changes to protobuf repo.

The effect of this PR will be to create a *broken* publish to BCR for each protobuf release. At least this red PR on BCR will be our indication that the patches need to be manually replayed there.

In parallel, starting with protocolbuffers#16013 I'll apply as many of those patches to the protobuf repo as possible. That will reduce the manual effort for each release.

Replaces protocolbuffers#14565 which originated from my fork so the tests wouldn't run.

Closes protocolbuffers#16014

COPYBARA_INTEGRATE_REVIEW=protocolbuffers#16014 from protocolbuffers:bcr e17d9c8
PiperOrigin-RevId: 615026796
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.

3 participants