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

Minor improvements #2651

Merged
merged 2 commits into from
May 3, 2024
Merged

Minor improvements #2651

merged 2 commits into from
May 3, 2024

Conversation

iamluc
Copy link
Contributor

@iamluc iamluc commented Apr 30, 2024

Description

Update CONTRIBUTING.md + fix the cbindgen Makefile target not working without installing cbindgen manually.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@iamluc iamluc force-pushed the luc/minor-improvements branch from fbbce21 to e6818f9 Compare May 2, 2024 09:22
Note: components-rs/*.h files generated by cbindgen cannot be generated
individually as they need to be deduplicated, so let's remove the
individual Makefile targets and regenerate the files only with the
"cbindgen" target.
@iamluc iamluc force-pushed the luc/minor-improvements branch from e6818f9 to fb94d7b Compare May 2, 2024 09:35

generate_cbindgen: components-rs/ddtrace.h components-rs/telemetry.h components-rs/sidecar.h components-rs/common.h
generate_cbindgen: cbindgen_binary # Regenerate components-rs/ddtrace.h components-rs/telemetry.h components-rs/sidecar.h components-rs/common.h
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some context:
Currently in our dev Docker container, running make cbindgen deletes the components-rs/*.h files.
Then, because the cbindgen command is not installed, the generation of the files is skipped silently.
Finally it fails in the deduplication step because the files are missing.

As the components-rs/*.h files cannot be generated individually because their declarations need to be deduplicated, I believe it's simpler to generate them in the same Makefile target.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree. And it's tidier :-)

@iamluc iamluc marked this pull request as ready for review May 2, 2024 12:00
@iamluc iamluc requested a review from a team as a code owner May 2, 2024 12:00
@iamluc iamluc merged commit 1d6cdcb into master May 3, 2024
553 of 555 checks passed
@iamluc iamluc deleted the luc/minor-improvements branch May 3, 2024 09:11
@bwoebi bwoebi added this to the 1.0.0 milestone May 27, 2024
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.

2 participants