Skip to content

Conversation

fw-immunant
Copy link
Contributor

The invocation here was out-of-date as of #554; the command now works as intended.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

How'd you determine these new invocation commands? The most up-to-date commands are in pdg/src/main.rs in mod tests, as those are the ones tested in cargo test and CI.

Could we just say to look there in the README.md?

@fw-immunant
Copy link
Contributor Author

This is basically what's in pdg.sh, but as you note also matches src/pdg/main,rs. I don't think suggesting to read the source (even with a pointer to where) is helpful for a user-facing README--we should have a shell command so people can get started quickly.

@fw-immunant fw-immunant merged commit b4b10e3 into master Jul 11, 2023
@fw-immunant fw-immunant deleted the fw/dyninstr-readme branch July 11, 2023 21:12
Comment on lines +3 to +4
${CARGO_TARGET_DIR:-../target/debug/c2rust-instrument} --metadata instrument.target/debug/metadata.bc -- build --manifest-path analysis/tests/misc/Cargo.toml
(cd analysis/tests/misc/instrument.target/debug; INSTRUMENT_BACKEND=debug INSTRUMENT_RUNTIME=bg METADATA_FILE=metadata.bc ./c2rust-analysis-tests-misc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this still might be broken, which is why I hadn't approved it earlier. The ${CARGO_TARGET_DIR:-../target/debug/c2rust-instrument} assumes the cwd is dynamic_instrumentation/ (I think, or at least some other directory), but analysis/tests/misc/Cargo.toml is relative to the repo root. Also, I don't remember if --metadata is supposed to be relative to the cwd or to the manifest dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a fix that works correctly with the repo root as cwd. Even when building the entire repo with cargo build in the repo root, c2rust-instrument ends up in the debug subdir of the cargo target directory; the other commands already expected to be there.

--metadata is relative to the cwd, which does mean that we need it to include the base path of the Cargo.toml.

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