nmattia opened PR #9580 from nmattia:nm-out-dir-relative
to bytecodealliance:main
:
Fixes #9553
When generating code,
cranelift-isle
records the path to the source files. These paths were sometimes absolute, meaning it kept a trace of the system it was run on.These changes here add helper functions in
Files
to try and strip the value of$OUT_DIR
(if set) from the file names.This also removes a log line (which included the full path of files) from
cranelift-codegen-meta
.
nmattia requested wasmtime-compiler-reviewers for a review on PR #9580.
nmattia requested cfallin for a review on PR #9580.
nmattia commented on PR #9580:
@cfallin here are some changes related to our discussion here.
This also removes a log line (which included the full path of files) from cranelift-codegen-meta.
About this :wait_one_second: I'm happy to remove it/submit it separately/make it relative.
@fitzgen you mentioned that, in the past, files were not printed with absolute paths. Is it worth investing some time to ensure this doesn't happen again in the future?
nmattia edited a comment on PR #9580:
@cfallin here are some changes related to our discussion here.
This also removes a log line (which included the full path of files) from cranelift-codegen-meta.
About this :wait_one_second: I'm happy to remove it/submit it separately/make it relative.
@fitzgen you mentioned that, in the past, files were not printed with absolute paths. Is it worth investing some time to ensure this doesn't happen again in the future?
Note: I originally changed the code to strip
CARGO_MANIFEST_DIR
, but it looks like the files were actually not in there; instead they were inOUT_DIR
. This might be an artifact of the build system used in my project; let me know if this makes sense.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I wonder if the build script that invokes cranelift-isle can read this instead. Cranelift-isle can technically be used to build arbitrary code.
Files::from_paths
could get a set of paths to make make input paths relative to, or alternatively a set of paths against which to resolve relative paths and then have the build script only pass the relative paths for individual isle files instead.
nmattia submitted PR review.
nmattia created PR review comment:
Sorry, not too familiar with the code/cranelift. Are you suggesting that we should make the paths relative here instead?
bjorn3 created PR review comment:
Yes. There is already a
make_isle_source_path_relative
function which gets used, but that only affects isle files that are part of the source rather than those that get generated by the build script itself. You did still have to separately pass a (set of) prefix(s) to join with the relative path to get a path that can be passed tofs::read
, but this prefix can be ignored when printing the source locations in the generated file.
bjorn3 submitted PR review.
nmattia submitted PR review.
nmattia created PR review comment:
only affects isle files that are part of the source rather than those that get generated by the build script itself.
Unrelated to this thread, but that explains why I had to use
OUT_DIR
instead ofCARGO_MANIFEST_DIR
!Yes.
That makes sense. I'll try to find some time later to update the PR.
github-actions[bot] commented on PR #9580:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:meta", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Last updated: Dec 23 2024 at 12:05 UTC