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

make bytearray work (again) #16691

Closed
wants to merge 1 commit into from

Conversation

jensbjorgensen
Copy link
Contributor

fix is pretty simple, just check if the type is bytearray and get the bytes if it is

addresses issue: #15911

@jensbjorgensen jensbjorgensen requested a review from a team as a code owner May 1, 2024 00:54
@jensbjorgensen jensbjorgensen requested review from anandolee and removed request for a team May 1, 2024 00:54
Copy link

google-cla bot commented May 1, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jensbjorgensen
Copy link
Contributor Author

I submitted a CLA, not sure why it says above I haven't

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 10, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 10, 2024
@anandolee anandolee self-assigned this May 10, 2024
@jensbjorgensen
Copy link
Contributor Author

ok, now I see what happened, I signed the CLA but the email address on the commit didn't match. so I've changed my commit now to have the email address that matches my (this) github account and push'ed -f, I just need to figure out how to get that commit to be the one associated with the pull request?

@jensbjorgensen
Copy link
Contributor Author

Ok I think maybe you have to re-approve then @anandolee ?

@jensbjorgensen
Copy link
Contributor Author

@anandolee sorry to bother, I amended my commit to reflect the email address associated with my github account, but I believe manual intervention may be required by you to let the PR move forward?

@honglooker honglooker added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 16, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 16, 2024
@jensbjorgensen
Copy link
Contributor Author

jensbjorgensen commented May 17, 2024

In the failing check (building Examples Win 7.1.1) it doesn't seem like the change I have would've caused this?

ERROR: C:/tnmkfdws/external/com_google_protobuf/src/google/protobuf/compiler/objectivec/BUILD.bazel:60:11: Compiling src/google/protobuf/compiler/objectivec/enum.cc [for tool] failed: (Exit 2): cl.exe failed: error executing CppCompile command (from target @@com_google_protobuf//src/google/protobuf/compiler/objectivec:objectivec) C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.39.33519\bin\HostX64\x64\cl.exe ... (remaining 1 argument skipped)
external/com_google_protobuf/src/google/protobuf/compiler/objectivec/enum.cc(20): fatal error C1083: Cannot open include file: 'google/protobuf/compiler/objectivec/text_format_decode_data.h': No such file or directory

@jensbjorgensen
Copy link
Contributor Author

hello, could someone take a look at this? I think the win7.1 test failure was unrelated to my change

@jensbjorgensen
Copy link
Contributor Author

so in the hopes of having automatic test pass (which seemed to fail for no reason related to my code) I've rebased and pushed this.

@jensbjorgensen
Copy link
Contributor Author

I think maybe this needs to be re-approved then @anandolee ?

@jensbjorgensen
Copy link
Contributor Author

@haberman it seems as though it is blocked (again), with "2 workflows awaiting approval", is this something you need to approve?

@haberman haberman added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 31, 2024
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label May 31, 2024
@jensbjorgensen
Copy link
Contributor Author

so now the checks have passed, looks like we just need an authorized user to merge this? @anandolee is that something that you do now?

@jensbjorgensen
Copy link
Contributor Author

@haberman is there maybe something you need to do to move the change along now?

@haberman haberman self-requested a review June 10, 2024 14:25
@jensbjorgensen
Copy link
Contributor Author

Hmmm... it just says "Merging is blocked" now, I guess someone authorized needs to click the "Merge pull request" button?

@haberman
Copy link
Member

The PR should not be merged directly. It will be applied as a CL internally and then propagated back to GitHub.

This workflow is triggered automatically when an approved PR passes all tests. I had forgotten that the most recent commit needs to be approved, which is why the workflow did not trigger before.

There is one more manual approval that must happen internally before the CL is submitted. But I think we should be able to get this in today. Thanks for your patience.

TinyTinni pushed a commit to TinyTinni/protobuf that referenced this pull request Jun 15, 2024
fix is pretty simple, just check if the type is bytearray and get the bytes if it is

addresses issue: protocolbuffers#15911

Closes protocolbuffers#16691

COPYBARA_INTEGRATE_REVIEW=protocolbuffers#16691 from jensbjorgensen:main 6249e62
PiperOrigin-RevId: 642623917
deannagarcia pushed a commit to deannagarcia/protobuf that referenced this pull request Jun 20, 2024
fix is pretty simple, just check if the type is bytearray and get the bytes if it is

addresses issue: protocolbuffers#15911

Closes protocolbuffers#16691

COPYBARA_INTEGRATE_REVIEW=protocolbuffers#16691 from jensbjorgensen:main 6249e62
PiperOrigin-RevId: 642623917
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.

4 participants