Build refactoring - cbindgen #313
Conversation
github-actions
bot
added
profiling
r1viollet
force-pushed
the
r1viollet/cbindgen_in_packages
branch
2 times, most recently
from
c7ee4cc to
02f1292
Compare
r1viollet
force-pushed
the
r1viollet/cbindgen_in_packages
branch
2 times, most recently
from
2a26f0c to
4b991c5
Compare
Ensure cbindgen is included in the package's build steps. The motivation is to remove logic from the shell scripts.
r1viollet
force-pushed
the
r1viollet/cbindgen_in_packages
branch
from
4b991c5 to
72533ef
Compare
…bindgen_in_packages
…bindgen_in_packages
Ensure we re-run cargo build scripts if cbindgen.toml file is udpated
…bindgen_in_packages
ddcommon-ffi/build.rs
Outdated
| let crate_dir = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap()); | ||
| let header_name = "common.h"; | ||
| let output_base_dir = env::var("DESTDIR").ok(); // Use `ok()` to convert Result to Option | ||
|
|
||
| generate_header(crate_dir, header_name, output_base_dir.as_deref()); | ||
| println!("cargo:rerun-if-env-changed=DESTDIR"); |
There was a problem hiding this comment.
We have the exact same code for all three crates, with the only difference being the header name. Maybe worth factoring into a function?
There was a problem hiding this comment.
I have mixed feelings about hiding some of this behaviour :-)
I added the magical function all the same.
build-common/src/lib.rs
Outdated
| cargo_target_path.join("include/datadog/").join(header_name) | ||
| } else { | ||
| // If relative, adjust the path accordingly. we are in a crate, so get back to top level | ||
| let adjusted_path = Path::new("..").join(cargo_target_path); |
There was a problem hiding this comment.
is this guaranteed to work? What if the command is run deeper in the crate?
There was a problem hiding this comment.
No.
However I can not think of a way to magically infer where the headers should be placed.
I looked into using the OUT_DIR variable, but that also has different depths depending on build parameters.
So I think this being not perfect still guarantees that it works in the use cases we have today.
Happy to reconsider if you find a different logic that works well.
There was a problem hiding this comment.
If we want the root of the workspace, can we try https://doc.rust-lang.org/cargo/commands/cargo-locate-project.html#options
There was a problem hiding this comment.
Here is a new iteration using your proposal.
…bindgen_in_packages
…ibdatadog into r1viollet/cbindgen_in_packages
Add a function to read configuration from env vars and calls into the header generation functions.
When DESTDIR is set, ensure we use the cargo target path to compute absolute paths.
…ative paths Use the locate-project feature to find the root of the project.
|
|
||
| /// Configure the header generation using environment variables. | ||
| /// call into `generate_header` with the appropriate arguments. | ||
| /// |
There was a problem hiding this comment.
I can still find ways to break this, but I feel we already improve the state of the build.
What does this PR do?
Ensure cbindgen is included in the package's build steps. By removing logics from the shell scripts, we can make build steps more modular.
This is step 1 of moving towards a common "feature" driven build system.
Motivation
You can refer to the RFC around features
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.
For Reviewers
@DataDog/security-design-and-guidance.