Skip to content
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

Stop using /dev/null for silent ui for WASI platform #5703

Merged

Conversation

kateinoigakukun
Copy link
Contributor

@kateinoigakukun kateinoigakukun commented Jul 10, 2022

What was the end-user or developer problem that led to this PR?

WASI doesn't guarantee that /dev/null is present.
So before this patch, we needed to mount host's /dev directory to WASI guest process to avoid ENOTCAPABLE error while require "bundler/setup"

Here is how to reproduce the problem.

# Download prebuilt ruby
curl -LO https://github.com/ruby/ruby.wasm/releases/download/2022-05-11-a/ruby-head-wasm32-unknown-wasi-full.tar.gz
tar xfz ruby-head-wasm32-unknown-wasi-full.tar.gz

# Install the same version of native ruby to avoid bundler version mismatch in "BUNDLED WITH" of Gemfile.lock
rbenv install 3.2.0-dev
rbenv local 3.2.0-dev

# Setup Gemfile
bundle init
bundle add syntax_tree

# Install gems in vendor/bundle to mount it on WASI
bundle config set --local path 'vendor/bundle'
bundle install

cat <<EOS > my_app.rb
require "bundler/setup"
require "syntax_tree"

pp SyntaxTree.parse("1 + 1")
EOS

# Options:
# --mapdir /usr::./head-wasm32-unknown-wasi-full/usr    Map ruby directory
# --mapdir /root::./                                    Map user script directory
# --env BUNDLE_GEMFILE=/root/Gemfile                    Tell bundler where the Gemfile located because WASI has no PWD concept
wasmtime run \
  --mapdir /usr::./head-wasm32-unknown-wasi-full/usr \
  --mapdir /root::./ \
  --env BUNDLE_GEMFILE=/root/Gemfile \
  head-wasm32-unknown-wasi-full/usr/local/bin/ruby -- /root/my_app.rb
/usr/local/lib/ruby/3.2.0+1/rubygems/user_interaction.rb:621:in `initialize': Capabilities insufficient @ rb_sysopen - /dev/null (Errno::ENOTCAPABLE)
        from /usr/local/lib/ruby/3.2.0+1/rubygems/user_interaction.rb:621:in `open'
        from /usr/local/lib/ruby/3.2.0+1/rubygems/user_interaction.rb:621:in `initialize'
        from /usr/local/lib/ruby/3.2.0+1/bundler/ui/rg_proxy.rb:11:in `initialize'
        from /usr/local/lib/ruby/3.2.0+1/bundler.rb:91:in `new'
        from /usr/local/lib/ruby/3.2.0+1/bundler.rb:91:in `ui='
        from /usr/local/lib/ruby/3.2.0+1/bundler.rb:87:in `ui'
        from /usr/local/lib/ruby/3.2.0+1/bundler/setup.rb:10:in `<top (required)>'
        from <internal:/usr/local/lib/ruby/3.2.0+1/rubygems/core_ext/kernel_require.rb>:85:in `require'
        from <internal:/usr/local/lib/ruby/3.2.0+1/rubygems/core_ext/kernel_require.rb>:85:in `require'
        from /root/my_app.rb:2:in `<main>'

If you add --mapdir /dev::/dev option to wasmtime command as a workaround, it runs expectedly.

What is your fix for the problem, implemented in this PR?

This patch removes the use of /dev/null in Gem::SilentUI to avoid the workaround.

Make sure the following tasks are checked

@welcome
Copy link

welcome bot commented Jul 10, 2022

Thanks for opening a pull request and helping make RubyGems and Bundler better! Someone from the RubyGems team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use GitHub Actions to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of GitHub Actions in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #rubygems or #bundler channel on Slack.

For more information about contributing to the RubyGems project feel free to review our CONTRIBUTING guide

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This sounds good to me. I'd like to add a test to simulate what's happening in WASI and cover this change, but I understand it could be tricky. Only nit is, could we move the initialize method to still be the first method in the class, like it's done in other classes in this file?

@kateinoigakukun
Copy link
Contributor Author

OK, I've added a test by faking File.open and adjusted coding-style

WASI doesn't guarantee that `/dev/null` is present.
So without this patch, we needed to mount host's `/dev` directory to WASI
guest process to avoid `ENOTCAPABLE` error while `require "bundler/setup"`
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thank you!

I rebased and squashed both commits (to not break future bisections), and will be merging as soon as CI passes.

@deivid-rodriguez deivid-rodriguez merged commit 8039b00 into rubygems:master Jul 17, 2022
deivid-rodriguez added a commit that referenced this pull request Jul 27, 2022
Stop using `/dev/null` for silent ui for WASI platform

(cherry picked from commit 8039b00)
@kateinoigakukun kateinoigakukun deleted the katei/wasm-slient-io branch December 8, 2023 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants