Skip to content

Conversation

@haberman
Copy link
Member

@haberman haberman commented Sep 3, 2022

Background

The Ruby C extension embeds an "amalgamation" of the upb sources into ruby-upb.c and ruby-upb.h. This amalgamation is a .c/.h file pair that concatenates all the sources from upb into a single .c and .h, similar to the SQLite amalgamation: https://www.sqlite.org/amalgamation.html

Prior to this PR, the amalgamation was only updated on an ad hoc basis, whenever someone decided to do it. This meant that the amalgamation was often months behind the canonical sources in https://github.com/protocolbuffers/upb.

PR Summary

This PR adds a staleness test, so that ruby-upb.c and ruby-upb.h will always be compared against the amalgamation as generated from the upb repo. If the amalgamation is out of date, the test will fail and the user will need to run:

$ bazel-bin/ruby/test_amalgamation_staleness --fix

Initially the PR contains a patch file against upb that makes generated_file_staleness_test() and amalgamate.py work properly across repos. However this will be submitted separately to the upb repo, and upb.patch will be reverted before merging this PR.

ruby/BUILD.bazel Outdated
cmd = "cp $< $@",
)

generated_file_staleness_test(
Copy link
Member

Choose a reason for hiding this comment

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

If this staleness test fails what are the mitigation steps? If the upb repo is the source of truth and nobody is editing these directly, this makes sense but might warrant a comment

Copy link
Member Author

Choose a reason for hiding this comment

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

If the staleness test fails it will tell you how to fix it. It will output a message like:

To fix run THIS command:
  bazel-bin/cmake/test_generated_files --fix

The expectation is that this will (temporarily) fail whenever we update our protobuf->upb dep to point to a newer commit:

commit = "470f06cccbf26f98dd2df7ddecf24a78f140fe11",

Copy link
Member

Choose a reason for hiding this comment

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

I guess my point is more that this strategy only works for 1 directional changes. If changes are coming from both sides this won't work

genrule(
name = "copy_ruby_amalgamation_c",
srcs = ["@upb//:ruby-upb.c"],
outs = ["generated-in/ext/google/protobuf_c/ruby-upb.c"],
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to allow the staleness test to more compactly support this case (i.e. just pass it the @upb targets). These genrules seem a bit hacky

@haberman haberman merged commit d3995ec into protocolbuffers:main Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants