@markdroth Got your message over at #23307.

I recently merged #23540, which updated the PHP Artifact test (to be run per pull request), which should catch these types of build issues for PHP. Do you mind rebasing this PR on top of the latest master? Then the "Artifact Build Linux (internal CI)" test should fail with the PHP build issue described in #23307, and we can iterate from there.

As it stands, the Artifact test should fail because the pair of filenames like these:

src/core/ext/upb-generated/envoy/annotations/deprecation.upb.c \
src/core/ext/upb-generated/envoy/annotations/deprecation.upbdefs.c \

The PHP build process will turn them into the same Makefile target, which would fail to compile.

@nicolasnoble I guess you are still working on a different strategy for exporting these symbols? Let me know how I can help. Thanks!

@stanley-cheung I've merged master.

I'm not sure that the problem that Nico is working on is directly related to the filename problem. I think the filename problem will have to be fixed independently, since there will definitely be cases where we'll need both the *.upb.c and *.upbdefs.c files in future. That will be needed as part of #23122, which we'll want to do towards the end of this quarter.

Actually, I guess the filename problem doesn't really have anything to do with #23122. It will be needed for this PR.

But I think that what Nico is working on will not address that. He was working on having upb export an API for descriptor.proto specifically, but that does not solve the filename problem for any of the other files.

Distribtest on master should now be green (just merged two fixes), so I think the only remaining distribtest failure would be the PHP distribtests failure:
(should be the same error as also reported by the updated PHP artifact build: https://source.cloud.google.com/results/invocations/bfb9364c-afac-4be5-b402-e6ce59266823/targets/github%2Fgrpc/tests)

Of course we'll still want to have an adhoc https://fusion.corp.google.com/projectanalysis/summary/KOKORO/prod%3Agrpc%2Fcore%2Fexperimental%2Fgrpc_build_artifacts_multiplatform run after we think we fixed the PHP distribtest.

Ad php artifact/distribtest breakage:
It complains /tmp/pear/temp/grpc/src/core/ext/filters/client_channel/xds/xds_api.h:31:23: fatal error: upb/def.hpp: No such file or directory, but package.xml (generated from the build*.yaml files) doesn't contain def.hpp at all.
The likely reason is that src/upb/gen_build_yaml.py also doesn't mention def.hpp at all.
I think there's a good chance that after adding def.hpp somewhere here https://github.com/grpc/grpc/pull/23533/files#diff-2125234f3dcd991b41d66e4c91cdcd2bR66 will fix the problem?

Ad php artifact/distribtest breakage:
It complains /tmp/pear/temp/grpc/src/core/ext/filters/client_channel/xds/xds_api.h:31:23: fatal error: upb/def.hpp: No such file or directory, but package.xml (generated from the build*.yaml files) doesn't contain def.hpp at all.
The likely reason is that src/upb/gen_build_yaml.py also doesn't mention def.hpp at all.
I think there's a good chance that after adding def.hpp somewhere here https://github.com/grpc/grpc/pull/23533/files#diff-2125234f3dcd991b41d66e4c91cdcd2bR66 will fix the problem?

This is just the first layer - after adding that file to package.xml will get past this particular error, but the much bigger problem is after that, we run into this issue with the filenames I talked about earlier in this thread, which has the detailed investigation in #23307.

So what we need to solve is that 2nd problem with the filenames. I'd like to get Nico's input on this.

Ad php artifact/distribtest breakage:
It complains /tmp/pear/temp/grpc/src/core/ext/filters/client_channel/xds/xds_api.h:31:23: fatal error: upb/def.hpp: No such file or directory, but package.xml (generated from the build*.yaml files) doesn't contain def.hpp at all.
The likely reason is that src/upb/gen_build_yaml.py also doesn't mention def.hpp at all.
I think there's a good chance that after adding def.hpp somewhere here https://github.com/grpc/grpc/pull/23533/files#diff-2125234f3dcd991b41d66e4c91cdcd2bR66 will fix the problem?

This is just the first layer - after adding that file to package.xml will get past this particular error, but the much bigger problem is after that, we run into this issue with the filenames I talked about earlier in this thread, which is detailed in #23307.

So what we need to solve is that 2nd problem with the filenames. I'd like to get Nico's input on this.

Thanks for the clarification.

I've added the missing dependency for def.hpp.

Yep, after adding def.hpp, we get back to the same state in #23307, with the PHP build failing with this error in "Artifact Build Linux (internal CI)":

https://source.cloud.google.com/results/invocations/db386d88-c722-4e9e-b887-ff5759f3db81/targets/grpc%2Fcore%2Fpull_request%2Flinux%2Fgrpc_build_artifacts/log

src/core/ext/upb-generated/udpa/annotations/.libs/status.o:(.data.rel.local+0x0): multiple definition of `udpa_annotations_status_proto_upbdefinit'
src/core/ext/upb-generated/udpa/annotations/.libs/status.o:(.data.rel.local+0x0): first defined here
src/core/ext/upb-generated/validate/.libs/validate.o:(.data.rel.local+0x0): multiple definition of `validate_validate_proto_upbdefinit'
src/core/ext/upb-generated/validate/.libs/validate.o:(.data.rel.local+0x0): first defined here

Triggered adhoc artifact/packages/distribtest flow: sponge/8aec0801-1e1d-42f3-be35-b7950dd9d5ac (I'll check the results later).

sponge/8aec0801-1e1d-42f3-be35-b7950dd9d5ac

I checked the results of that build flow and all the distribtests (including the python distribtests that were previously broken) are passing except of the PHP distribtest (tracked by #23307).

@jtattermusch Does that include the cfstream tests that were previously failing?

@jtattermusch Does that include the cfstream tests that were previously failing?

Nope, just the artifact / packages /distribtest flow. Everything else that was broken needs to be tested separately.

I am thinking about a possible approach like this:

• for PHP build only, at artifact build time, rename the *.upbdefs.c files to *_upbdefs.c.
• rename all the references like all the #include from *.upbdefs.c files to *_upbdefs.c as well

The last time I brought this up, Nico seems to think that this approach may not work and that he is working on a different approach. So I did not proceed with experimenting with this.

Do you think I should experiment with this? Do the file names play into how the symbols are named within the file?

I suspect that the filenames are not so important for the .c file, but they are for the .h file, because they get included from other files using the original name.

I suspect that the filenames are not so important for the .c file, but they are for the .h file, because they get included from other files using the original name.

@markdroth This is looking promising.

I added a script, for the purpose of the PHP build only, to temporarily rename all the *.upbdefs.c|h files into *_upbdefs.c|h, and update all the #include references from other files. I am able to pass the Artifact Build test now. Here's an adhoc run (for just PHP), and another adhoc run (all lang).

I created PR #23614, which is basically this PR, plus this PHP fix 4f6d6d8. However, I am unable to resolve the conflicts with master myself. So the PR is not running any tests yet.

Mark, can you resolve the conflicts of this PR one more time please? There are some conflicts in src/core/ext/filters/client_channel/xds/xds_api.cc and tools/codegen/core/gen_upb_api.sh that I wasn't able to resolve. After that, I can re-build my PHP fix on top of this, and then I can run the full suite of PR tests to make sure we can move this forward. If everything looks good, I can submit a PR to add my commit to your branch here. Thanks!

After #23904 is merged, you can recreate a slimmer version of this PR on top of upstream/master. Please try to keep generated and handwritten changes in separate commits so that the resulting PR is easier to review.

The distribtests seem to be working now:

https://sponge.corp.google.com/target?id=3321a5f3-f182-4829-be3c-6beb8c790afc&target=grpc%2Fcore%2Fexperimental%2Fgrpc_build_artifacts_multiplatform&searchFor=

I'll start looking into what internal changes are needed to cherry-pick this.

Known issues: #23597 #18111

Not sure what went wrong with the bad_ssl_cert_test in MSAN, but it looks like it failed to find the server binary. I suspect it's some kind of infrastructure flake.

I think this is ready to merge. I'll do that as soon as the current import goes in.

Unfortunately, this also breaks rust build maintained at https://github.com/tikv/grpc-rs. The errors are also something like "multiple definitions"

/usr/bin/ld: /grpc-rs/target/debug/build/grpcio-sys-6c974093f07f78a9/out/build/libgrpc.a(struct.upb.c.o):(.data.rel.ro.local.google_protobuf_Value_msginit+0x0): multiple definition of `google_protobuf_Value_msginit'; /grpc-rs/target/debug/build/grpcio-sys-6c974093f07f78a9/out/build/libupb.a(struct.upb.c.o):(.data.rel.ro.local.google_protobuf_Value_msginit+0x0): first defined here

I can see from the comments that you get around the problem by renaming files at build time, which seems hacky to me. Do you have any other suggestions? Or can we just disable upb completely?

I'm not sure how you're building grpc-rs or how it's integrated with our existing build system, but @veblush may be able to help you figure out how to make this work.

As a work-around, you can't support xDS without upb, but if you're using bazel, there is a way to build without xDS support at all. See #23658.

The build error output can be found at github action log. Basically, it compiles grpc using cmake and then link every necessary static library including libgrpc.a and libupb.a to a binary.

Not supporting xDS can be a workaround indeed, thanks for your advice. Just out of curious, if changing file names can fix build issues, then why not just renaming every thing the time it's generated so it works for all?

If you're using cmake, there probably isn't an easy way to build without xDS support; that works for bazel only. Also, I suspect that gRPC rust users will actually want to use xDS; we had at least one person asking about this at the last community meeting.

I haven't looked at the full build log, but based on the error message you pasted above, I suspect this has nothing to do with renaming files. The file-renaming thing was a PHP-specific issue, not likely to affect other languages.

In any case, while this PR may have triggered the problem, it was merged 3 months ago, and we're not going to roll it back at this point, so I think your build problem is a separate issue. Please file an issue about it and tag @veblush on it, since he may be able to help you figure this out.