Skip to content

DO NOT MERGE: fix PHP build issue with upb textformat#23628

Closed
jtattermusch wants to merge 10 commits intogrpc:masterfrom
jtattermusch:xds_logging
Closed

DO NOT MERGE: fix PHP build issue with upb textformat#23628
jtattermusch wants to merge 10 commits intogrpc:masterfrom
jtattermusch:xds_logging

Conversation

@jtattermusch
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch commented Jul 27, 2020

Based on #23533 - an attempt to solve the PHP artifact/distribtest issues.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jul 27, 2020

This is an improved/alternative version of #23614

  • doesn't need any preprocessing before building PHP (prepare_pecl_extension.sh), as the build files already include the .upbdefs.c files with a patched name
  • just ignore the "upbdefs.h" files, it doesn't seem to be necessary to update them (only the .c files generate the conflicting object files?).

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jul 27, 2020

@stanley-cheung @markdroth WDYT?

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jul 27, 2020

Depending on the nature of the PHP build issue, there might be one alternative approach - currently all the UPB generated files go under the "upb-generated" directory. It would be pretty easy to make all the .upbdefs.c files end up in a different directory (say "upbdefs-generated"), which may prevent the PHP problem (the upb.c and .upbdef.c files will be in different directory which may avoid the collision) and we would't need to patch the file names of the generated files.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Jul 27, 2020

In a local experiment (ran with DOCKERHUB_ORGANIZATION=grpctesting tools/run_tests/task_runner.py -f php linux artifact) the PHP artifact build has succeeded.

This was referenced Jul 27, 2020
Closed
Closed
markdroth
markdroth reviewed Jul 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Will this approach make it harder to move forward with #23122 when the time comes?

tools/codegen/core/gen_upb_api.sh


# In PHP build Makefile, the files with .upb.c suffix collide .upbdefs.c suffix due to a PHP buildsystem bug.
# Work around this by changing the generated suffix from ".upbdefs.c" to "_ubpdefs.h".
Copy link
Copy Markdown
Member

@markdroth markdroth Jul 27, 2020

Choose a reason for hiding this comment

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

I think you mean "_upbdefs.c", not "_upbdefs.h".

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Aug 14, 2020

Superseded by #23835.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markdroth markdroth markdroth left review comments

@sergiitk sergiitk Awaiting requested review from sergiitk sergiitk will be requested when the pull request is marked ready for review sergiitk is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jtattermusch @markdroth