Stream: git-wasmtime

Topic: wasmtime / PR #9580 Make source links relative to OUT_DIR

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 11:26):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 11:26):

nmattia requested wasmtime-compiler-reviewers for a review on PR #9580.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 11:26):

nmattia requested cfallin for a review on PR #9580.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 11:28):

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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 11:29):

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 in OUT_DIR. This might be an artifact of the build system used in my project; let me know if this makes sense.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 11:53):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 11:53):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 12:28):

nmattia submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 12:28):

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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 12:32):

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 to fs::read, but this prefix can be ignored when printing the source locations in the generated file.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 12:32):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 12:48):

nmattia submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 12:48):

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 of CARGO_MANIFEST_DIR !

Yes.

That makes sense. I'll try to find some time later to update the PR.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 07 2024 at 14:44):

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:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Last updated: Nov 22 2024 at 16:03 UTC