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

Address compilation issues with auto generated amqp files #1820

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

ddelnano
Copy link
Member

Summary: Address compilation issues with auto generated amqp files

When reviewing #1816, I realized that there was drift from the amqp template files and the resulting code. The updated stitcher interface was propagated to the auto generated files, but not the template itself. As for the other changes, I wasn't able to track down why they didn't exist in the template file but these changes were necessary to get the code compiling again.

Once this is merged, #1816 should be easy to merge afterwards.

Relevant Issues: #1816

Type of change: /kind cleanup

Test Plan: amqp trace bpf test passes

@ddelnano ddelnano requested a review from a team as a code owner January 16, 2024 16:45
@ddelnano ddelnano force-pushed the ddelnano/fix-amqp-code-generation branch from 7cc8b80 to ba67f2d Compare January 16, 2024 16:46
@ddelnano ddelnano force-pushed the ddelnano/fix-amqp-code-generation branch from ba67f2d to f4f68f9 Compare January 16, 2024 17:12
Signed-off-by: Dom Del Nano <[email protected]>
@ddelnano ddelnano requested a review from a team January 18, 2024 20:49
@vihangm
Copy link
Member

vihangm commented Jan 23, 2024

This seems like a good candidate to implement using diff_test and write_file from bazel_skylib. This will ensure that these files will never fall out of sync.

A typical impl looks like follows:

genrule(
    name = "gen_files",
    srcs = [],
    outs = [<generated_files>],
    cmd = """
        $(location /src/stirling/source_connectors/socket_tracer/protocols/amqp/amqp_code_generator:amqp_code_gen_main) \
        --run
    """,
    tools = ["/src/stirling/source_connectors/socket_tracer/protocols/amqp/amqp_code_generator:amqp_code_gen_main"],
)

diff_test(
    name = "check_files",
    failure_message = "Please run: bazel run <full target to :gen_files>",
    file1 = "<repo_file>",
    file2 = "<generated_file>",
)

write_file(
    name = "gen_update",
    out = "update.sh",
    content = [
        "#!/bin/bash",
        "cd $BUILD_WORKSPACE_DIRECTORY",
        "cp -fv bazel-bin/<generated_file> <repo_file>",
    ],
)

sh_binary(
    name = "update_files",
    srcs = ["update.sh"],
    data = ["<generated_files>"],
)

This generates a "test" that fails if the generated file differs from the file in the repo and a update rule that lets you bazel run a target that updates the file in the repo.

See https://github.com/bazelbuild/rules_go/blob/30099a6add3c43706b4eec82b773b78310874935/docs/doc_helpers.bzl#L19 if you want an idea for how someone strings this together.

Copy link
Member

@vihangm vihangm left a comment

Choose a reason for hiding this comment

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

Merging to unblock but IMO it's probably worth spending the time to write the diff_test at the very least.

@vihangm vihangm merged commit 4018018 into pixie-io:main Jan 23, 2024
34 checks passed
@ddelnano ddelnano deleted the ddelnano/fix-amqp-code-generation branch January 23, 2024 05:58
@ddelnano
Copy link
Member Author

Thanks for the suggestion. I have a work in progress change that is close to what you've described.

One of the challenges is that our non arc lint rules apply more changes than clang-format (ArcanistPixieTextLinter.php enforces files ending in new lines). What this means is that even if the clang-format invocation within amqp code generation used the correct .clang-format file, the resulting code will ultimately fail arc lint.

I'm in the process of working through that and will have a PR open once that's addressed.

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.

3 participants