Stream: git-wasmtime

Topic: wasmtime / PR #8871 component-macro: normalize the path p...

Wasmtime GitHub notifications bot (Jun 25 2024 at 09:18):

iawia002 requested elliottt for a review on PR #8871.

Wasmtime GitHub notifications bot (Jun 25 2024 at 09:18):

iawia002 requested wasmtime-core-reviewers for a review on PR #8871.

Wasmtime GitHub notifications bot (Jun 25 2024 at 09:18):

iawia002 opened PR #8871 from iawia002:normalize-path to bytecodealliance:main:

I encountered a file path issue while using the wasmtime::component::bindgen! macro, I used a relative file path (e.g., ../wit) in the path parameter, and then I received an error message:

failed to read path for WIT [/Users/code/test-component/examples/../wit]

The file path in the error message was quite confusing, in reality, the path that was not found should be /Users/code/test-component/wit. The relative file path in the error message made it hard for me to immediately understand which directory wasmtime::component::bindgen! was using to look for WIT files. Therefore, this patch normalizes the path before resolving WIT files.

Error message before:

failed to read path for WIT [/Users/code/test-component/examples/../wit]

after:

failed to read path for WIT [/Users/code/test-component/wit]

The wit_bindgen::generate! macro has the same issue. If this patch is reasonable, I will make the same changes to wit_bindgen::generate! afterward.

Wasmtime GitHub notifications bot (Jun 25 2024 at 18:34):

alexcrichton commented on PR #8871:

Thanks for the PR! Personally though I find path manipulation very tricky/subtle. For example in the presence of symlinks I'm not sure that this implementation is correct. In lieu of that perhaps std::fs::canonicalize could be used instead? If that returns Ok then the resulting path can be used and if it returns Err then the original path can be used as-is to probably return an error somewhere else.

Wasmtime GitHub notifications bot (Jun 26 2024 at 02:07):

iawia002 commented on PR #8871:

Thanks for the review, I agree that path manipulation is tricky, std::fs::canonicalize is more of a compromise solution. It can handle some scenarios (such as when the path itself exists but the wit file does not). However, if the path itself does not exist, the error message will still be the same as it is now. But I believe this is acceptable because we would rather not incorrectly normalize the path in some corner cases.

Wasmtime GitHub notifications bot (Jun 26 2024 at 02:07):

iawia002 updated PR #8871.

Wasmtime GitHub notifications bot (Jun 26 2024 at 17:16):

alexcrichton submitted PR review:

Oh that's a good point yeah, I had forgotten about that. Seems like a reasonable compromise though!

Wasmtime GitHub notifications bot (Jun 26 2024 at 17:30):

alexcrichton merged PR #8871.

Last updated: Oct 23 2024 at 20:03 UTC