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.

Sign up for GitHub

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

Jump to bottom

refactor: remove protobuf_workspace #181

Merged
merged 7 commits into from
Oct 21, 2023
Merged

refactor: remove protobuf_workspace #181

merged 7 commits into from
Oct 21, 2023

Conversation

thesayyn
Copy link
Collaborator

@thesayyn thesayyn commented Oct 4, 2023 •
edited by comius
Loading

  • removes protobuf workspace
  • removes dependency on protobuf repository.

currently, there are no prebuilt binaries available, it's built from the source using the protobuf workspace. Will add the toolchain for prebuilt binaries in a follow-up

RELNOTES[INC]: protobuf_repo was removed

@thesayyn thesayyn requested review from lberki and comius as code owners October 4, 2023 19:07
comius
comius requested changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@comius comius left a comment

Choose a reason for hiding this comment

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

Thanks for this change!

Please also fix the CI problems.

dev_deps.bzl Outdated Show resolved Hide resolved
dev_deps.bzl Show resolved Hide resolved
dev_deps.bzl Show resolved Hide resolved
proto/repositories.bzl Outdated Show resolved Hide resolved
tools/mirror_releases.sh Outdated Show resolved Hide resolved
dev_deps.bzl Show resolved Hide resolved
@thesayyn thesayyn requested a review from a team as a code owner October 6, 2023 16:46
comius
comius requested changes Oct 6, 2023
View reviewed changes
dev_deps.bzl Show resolved Hide resolved
dev_deps.bzl Show resolved Hide resolved
tools/mirror_releases.sh Outdated Show resolved Hide resolved
@fowles
Copy link

fowles commented Oct 6, 2023

quick heads up that I renamed the branch master to main, so you may need to kick this a bit if something goes funny there

comius
comius requested changes Oct 9, 2023
View reviewed changes
Copy link
Collaborator

@comius comius left a comment

Choose a reason for hiding this comment

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

Please also fix CI failures.

dev_deps.bzl Show resolved Hide resolved
dev_deps.bzl Show resolved Hide resolved
MODULE.bazel Show resolved Hide resolved
proto/private/versions.bzl Outdated Show resolved Hide resolved
tools/mirror_releases.sh Outdated Show resolved Hide resolved
@comius
Copy link
Collaborator

comius commented Oct 9, 2023

Please also fix CI failures.

In case you can't access them, let me know. The failures are:

run the following command in your workspace:<br/><pre><code>buildifier MODULE.bazel WORKSPACE dev_deps.bzl proto/BUILD proto/private/BUILD proto/private/versions.bzl</code></pre>

@thesayyn
Copy link
Collaborator Author

Please also fix CI failures.

In case you can't access them, let me know. The failures are:

run the following command in your workspace:<br/><pre><code>buildifier MODULE.bazel WORKSPACE dev_deps.bzl proto/BUILD proto/private/BUILD proto/private/versions.bzl</code></pre>

done

@thesayyn
Copy link
Collaborator Author

Beware that there are tests that depend on protobuf repo. should I remove those as well?

@comius
Copy link
Collaborator

comius commented Oct 11, 2023

Beware that there are tests that depend on protobuf repo. should I remove those as well?

Could you add a dev dependency to protobuf repo and update the test so that it uses sources?

@thesayyn
Copy link
Collaborator Author

Could you add a dev dependency to protobuf repo and update the test so that it uses sources?

19dbb3f can I take this change back then?

@comius
Copy link
Collaborator

comius commented Oct 11, 2023

Could you add a dev dependency to protobuf repo and update the test so that it uses sources?

19dbb3f can I take this change back then?

Yes, please do. Sorry for the trouble

@thesayyn
Copy link
Collaborator Author

@comius PTAL.

comius
comius approved these changes Oct 17, 2023
View reviewed changes
Copy link
Collaborator

@comius comius left a comment

Choose a reason for hiding this comment

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

There's still a buildifier warning, please run buildifier MODULE.bazel WORKSPACE dev_deps.bzl

@comius comius self-assigned this Oct 17, 2023
@comius comius added the ready to pull Required label for automatic import to Google label Oct 17, 2023
@comius
Copy link
Collaborator

comius commented Oct 19, 2023

There's still a buildifier warning, please run buildifier MODULE.bazel WORKSPACE dev_deps.bzl

Friendly ping.

Please also remove the toplevel BUILD file. I'll create that one in a separate PR, that follow licensing guidelines. (ATM that file breaks copybara)

@comius comius removed the ready to pull Required label for automatic import to Google label Oct 19, 2023
@comius comius added the ready to pull Required label for automatic import to Google label Oct 20, 2023
@comius comius removed the request for review from lberki October 20, 2023 08:25
@comius comius merged commit 1924117 into bazelbuild:main Oct 21, 2023
comius added a commit that referenced this pull request Oct 21, 2023
@comius
Merge pull request #181 from thesayyn:rm-protobuf-repo
c9731b8 
PiperOrigin-RevId: 575226356
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yannic Yannic Yannic left review comments

@alexeagle alexeagle alexeagle left review comments

@comius comius comius approved these changes

Assignees

@comius comius

Labels
ready to pull Required label for automatic import to Google
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thesayyn @fowles @comius @alexeagle @Yannic