rv ci: Use rv ruby, not system ruby #181
Conversation
System ruby probably shouldn't be trusted, this is rv after all, we should use rv ruby.
indirect
left a comment
indirect
left a comment
There was a problem hiding this comment.
this looks reasonable to me... maybe later we can invoke run directly rather than shelling out?
kaspth
left a comment
kaspth
left a comment
There was a problem hiding this comment.
Nice! I had a few questions, but feel free to skip if they're getting in the way.
crates/rv/src/commands/ci.rs
Outdated
| let mut child = std::process::Command::new("rv") | ||
| .args([ | ||
| "ruby", | ||
| "run", | ||
| &ruby_version.to_string(), | ||
| "--", | ||
| "-e", | ||
| "Gem.load_yaml; print Gem::SafeYAML.safe_load(ARGF.read).to_ruby", | ||
| ]) |
There was a problem hiding this comment.
Just a Rust question since I'm new: would it make sense to extract a Runner struct that we inject the ruby_version into and then do this?
pub fn run(&self, args) {
std::process::Command::new("rv").args(["ruby", "run", &self.ruby_version.to_string(), "--", ..args]).output()
}then we can pass the runner around instead of the plain ruby_version.
There was a problem hiding this comment.
Either one would work, but what is the advantage of doing your proposal?
There was a problem hiding this comment.
OK actually yeah, I like this, because otherwise we have to thread the config many function calls deep.
3 participants
System ruby probably shouldn't be trusted,
this is rv after all, we should use rv ruby.