-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added a quickfix for mounting relative directories #3818
Conversation
cd73d70
to
0a732a0
Compare
0a732a0
to
a8fc0fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
/// | ||
/// This isn't really a long-term solution because there is no guarantee what | ||
/// the current directory will be. The WASI spec also doesn't require the | ||
/// current directory to be part of the "main" filesystem at all, we really |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be mounting to a relative directory
Not quite sure what you mean there.
Mounting to a relative directory doesn't make sense, because as you mentioned, the relative directory is an ephemeral thing that always changes in the instance environment.
As I see it, mounting only makes sense with both absolute host and guest paths.
Auto-expanding a relative to an absolute path makes sense for convenience, but I don't see what else we should be doing.
// Sometimes "." won't be mounted so preopening will fail. | ||
builder.add_preopen_dir(".")?; | ||
|
||
if self.mapped_dirs.iter().all(|m| m.guest != ".") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens the user already specified a mount for /
?
Seems like we should check for that and error out otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine. This just makes sure .
points to the root directory on startup and doesn't affect any existing mounts on /
.
Description
This adds a test for #3794 and a temporary fix that will make
wasmer run --mapdir .:.
work again.The proper fix for #3794 will probably require an overhaul of the
virtual_fs::FileSystem
andWasiFs
layers, which isn't planned for the immediate future.