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

Deprecate artichoke-load-path and replace it with mezzaluna-feature-loader #2172

Open
lopopolo opened this issue Sep 8, 2022 · 4 comments
Open
Labels
A-crate-features Area: Compile-time features or attributes. A-filesystem Area: Filesystem access and implementations.

Comments

@lopopolo
Copy link
Member

lopopolo commented Sep 8, 2022

artichoke-load-path and mezzaluna-feature-loader both provide support for resolving Ruby sources from disk, memory, and using the RUBYLIB environment variable.

mezzaluna-feature-loader is meant to be a generalization of all of these loaders as well as an implementation of artichoke_backend::fs.

Work to be done:

  • Add implementation of in-memory-only loader to mezzaluna-feature-loader.
  • Add "hybrid" loader that combines in-memory, disk, and RUBYLIB loaders.
  • Replace artichoke_backend::fs with mezzaluna-feature-loader.
  • Simplify feature flags in artichoke and artichoke-backend to remove the ability to disable the RUBYLIB loader. Loader is either "full" or in-memory.
  • Remove artichoke-load-path code, Cargo.toml entry, and CI steps.
@lopopolo lopopolo added A-filesystem Area: Filesystem access and implementations. A-crate-features Area: Compile-time features or attributes. labels Sep 8, 2022
@lopopolo
Copy link
Member Author

lopopolo commented Apr 26, 2023

It looks like these (old) issues might be feasible once this work lands because mezzaluna-feature-loader tracks the set of loaded features:

This was referenced Apr 26, 2023
lopopolo added a commit that referenced this issue May 20, 2023
Add tests, add APIs, change error behavior.

Prep for: #2172.
@lopopolo
Copy link
Member Author

mezzaluna-feature-loader stores load paths as PathBuf but SHOULD guarantee that these load paths can be converted to bytes so they can be turned into an Array of Strings. On wasi and unix targets, Path and OsStr are freely convertible to bytes. On all other platforms, ensure load paths are valid UTF-8 with OsStr::to_str.

@lopopolo
Copy link
Member Author

I need to add a separate extension hook loader in mezzaluna_feature_loader::loaders.

@lopopolo
Copy link
Member Author

The existing disk loader in artichoke_backend::load_path::Native does not restrict CWD-relative requires to "explicit relative paths.

MRI

$ echo 'puts "here in the rubylib"' > a.rb
$ ruby -e 'require "a"'
<internal:/usr/local/var/rbenv/versions/3.2.2/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require': cannot load such file -- a (LoadError)
        from <internal:/usr/local/var/rbenv/versions/3.2.2/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
        from -e:1:in `<main>'
$ ruby -e 'require "./a"'
here in the rubylib
$ RUBYLIB=. ruby -e 'require "a"'
here in the rubylib

Artichoke

$ echo 'puts "here in the rubylib"' > a.rb
$ cargo run -q --bin artichoke -- -e 'require "a"'
here in the rubylib
$ cargo run -q --bin artichoke -- -e 'require "./a"'
here in the rubylib
$ RUBYLIB= cargo run -q --bin artichoke -- -e 'require "a"'
here in the rubylib

Root Cause

The Native loader directly feeds paths to File::open to check validity without first checking if they are explicit relative (and therefore meant to be resolved reltive to CWD rather than the load path).

pub fn resolve_file(&self, path: &Path) -> Option<PathBuf> {
if File::open(path).is_ok() {
Some(path.to_owned())
} else {
None
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-crate-features Area: Compile-time features or attributes. A-filesystem Area: Filesystem access and implementations.
Development

No branches or pull requests

1 participant