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

Revert "Revert "Adding xxhash as a subtree"" #25645

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

donnadionne
Copy link
Contributor

Adding xxhash as a subtree

Attempting to submit the same code after taking care of import issues.

@donnadionne donnadionne added the release notes: no Indicates if PR should not be in release notes label Mar 8, 2021
@donnadionne donnadionne marked this pull request as draft March 8, 2021 06:25
@jtattermusch
Copy link
Contributor

there's a bunch of failures and you'll also need a passing adhoc artifacts - packages - distribtest (distribtests were broken the last time) run before this can go in (see bullet point 2 here: https://github.com/grpc/grpc/tree/master/third_party#checklist-for-adding-a-new-third-party-dependency)

@donnadionne
Copy link
Contributor Author

there's a bunch of failures and you'll also need a passing adhoc artifacts - packages - distribtest (distribtests were broken the last time) run before this can go in (see bullet point 2 here: https://github.com/grpc/grpc/tree/master/third_party#checklist-for-adding-a-new-third-party-dependency)

started: https://fusion2.corp.google.com/invocations/8e76a837-d153-4677-987e-2a373e9df5ea/targets

@donnadionne donnadionne marked this pull request as ready for review March 9, 2021 06:13
@donnadionne donnadionne marked this pull request as draft March 9, 2021 06:15
@jtattermusch
Copy link
Contributor

there's a bunch of failures and you'll also need a passing adhoc artifacts - packages - distribtest (distribtests were broken the last time) run before this can go in (see bullet point 2 here: https://github.com/grpc/grpc/tree/master/third_party#checklist-for-adding-a-new-third-party-dependency)

started: https://fusion2.corp.google.com/invocations/8e76a837-d153-4677-987e-2a373e9df5ea/targets

That build seemed stuck for some reason, so I started another one https://sponge2.corp.google.com/e5a04e01-2ed9-4776-976f-528021b103ae

@donnadionne
Copy link
Contributor Author

donnadionne commented Mar 9, 2021

Linux and windows run from the previous ad-hoc run passed:
https://sponge2.corp.google.com/e5a04e01-2ed9-4776-976f-528021b103ae

mac run encountered "Tools failures" from previous ad-hoc runs; triggered another one after synching:
https://fusion2.corp.google.com/invocations/0046ece3-ddd0-4e84-ba7a-93836099cbb2/targets

The latest ad-hoc run passed!
https://fusion2.corp.google.com/invocations/0046ece3-ddd0-4e84-ba7a-93836099cbb2/targets

@donnadionne
Copy link
Contributor Author

Known issues: #25660 (interop test failures)

@donnadionne
Copy link
Contributor Author

kokoro failure: due to unable to access the commitsha that is not yet submitted

@donnadionne donnadionne marked this pull request as ready for review March 10, 2021 05:04
@donnadionne
Copy link
Contributor Author

All Required tests pass; failed test have explainations.

This is really the same change as last week; just now it's ready to be imported.

PTAL thank you!

@jtattermusch
Copy link
Contributor

@jtattermusch
Copy link
Contributor

jtattermusch commented Mar 10, 2021

FTR, as discussed I think the issue with python-dev distribtests is that the python "sdist" archive is missing the newly added xxhash.h header:

https://pantheon.corp.google.com/storage/browser/_details/grpc-testing-kokoro-prod/test_result_public/prod/grpc/core/master/linux/grpc_build_artifacts/16172/20210309-131926/github/grpc/artifacts/python_manylinux2010_x86_cp38-cp38/grpcio-1.37.0.dev0.tar.gz (and indeed, if you open the archive you'll see that third_party/xxhash/xxhash.h is not there).

Made a one line change in setup.py (similar fix as #24450: to allow grpcio to build with xxhash), re-running adhoc to test it out:
https://fusion2.corp.google.com/invocations/a7a23ae2-39a6-4d02-9f14-ca4dcf8ef25d/targets

This was not the right fix; setup.py restored now

@donnadionne donnadionne force-pushed the xxhash_second branch 2 times, most recently from 07aa917 to 59c5010 Compare March 10, 2021 19:20
@donnadionne
Copy link
Contributor Author

donnadionne commented Mar 11, 2021

Updated PYTHON-MANIFEST.in which fixed the grpc/core/experimental/grpc_distribtests_multiplatform: grpc/core/master/linux/grpc_distribtests:
https://source.cloud.google.com/results/invocations/dcd3d928-cf65-4fed-9cf0-d075eacaaed8/targets

Everything passed! https://fusion2.corp.google.com/invocations/75c8fbe4-d169-472e-86d3-098c8d4b813f/targets/grpc%2Fcore%2Fexperimental%2Fgrpc_distribtests_multiplatform;config=default/log

@donnadionne
Copy link
Contributor Author

Known issues: #25660 (interop test failures)

Ruby Failue is just a timeout, and this test has passed in previous runs before the 1 line change for Python.

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM (assuming you've tried the cherrypick into google3 and it worked fine).

@donnadionne donnadionne merged commit d3e97d9 into grpc:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants