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

Hybrid Rust (outside) + Bash (inside) #47

Merged
merged 19 commits into from
Jul 2, 2024
Merged

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Jun 12, 2024

This PR removes all Bash logic outside of the container and replaces it with Rust commands, see the README for more info. It removes all Ruby dependencies.

Superscedes #46

Rewrites the project from Bash to Rust (previously was rewritten from Ruby to Bash).

## Why Rust?

I bet you're thinking "Why not something simpler like bash or Ruby?" This library was originally written in Ruby and shelled out. That caused bootstrapping problems, for example when rolling out ARM support, the github action for installing Ruby did not yet support ARM so we had to re-write the logic in Bash (or bootstrap our own version of Ruby with bash). We chose to re-write the library in bash. So why not keep it in bash? Even though bash doesn't need "bootstrapping" authors rely on system tools and packages which may or may not already be installed, and may or may not vary between operating systems. For example GNU grep uses different arguments than BSD (mac) grep. So while there's not a "bash bootstrapping problem" installing dependencies and ensuring scripts work across multiple platforms is tricky. It's easy to write quick scripts, but hard to maintain and do well.

Don't you have a Rust bootstrapping problem now? As of Ruby 3.2 YJIT requires a Rust toolchain to support the `--enable-yjit` config flag, so Rust is already a requirement of a full Ruby install. That means that even if we didn't write our scripts in Rust, we still need to have it available on the system when we build Ruby anyway. It's also historically been an easy to install language.
Turns out, we don't need to re-write java properties parsing logic, an existing crate exists.
@schneems schneems force-pushed the schneems/riir-hybrid branch 4 times, most recently from 65d9c80 to fb42c18 Compare June 12, 2024 21:20
Outer scripts are still Rust, inner script is now bash
@schneems schneems force-pushed the schneems/riir-hybrid branch 2 times, most recently from 9bc079b to dd266d8 Compare June 13, 2024 15:23
@schneems schneems marked this pull request as ready for review June 13, 2024 15:30
@schneems schneems requested a review from a team as a code owner June 13, 2024 15:30
- Reduce logic duplication by moving the `./configure` options to an array
- Add some minimal comments
- Prettier command logging via trap instead of `set -o trace`
Previously this held the `make_ruby.rs` script and a shared set of tools. Now it only holds those tools.
Copy link
Contributor

@colincasey colincasey left a comment

Choose a reason for hiding this comment

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

I see no issues with this approach. It's nice to see the Ruby code eliminated and the amount of Bash scripting minimized. Mind you, I don't routinely have to do binary builds so maybe I'm not the person who should validate if the tradeoff between the Rust code vs. plain Bash scripting is worth it.

That said, what kind of approval are you looking for here @schneems?

@schneems
Copy link
Contributor Author

what kind of approval are you looking for here

The approving kind 😅.

Short: Compliance check.

Long: Honestly, "doesn't look actively malicious" is good enough. A rough scan for any "uh, this is clearly wrong logic" perhaps. Also worth noting, that I need the Heroku-24 ARM builds fix that's in here.

🙇🏻 This is frankly the kind of project that can be rewritten in a (long) weekend, so I'm not too worried about architecture mistakes or perfect code. If anything, I think I went TOO far with Rust (i.e., error handling, etc. instead of expect), but that's fine.

If we have better patterns in the future I can adopt them. I'm open to ideas. For example: I chose to use different binaries for the different commands (build/check) as I felt not having to share an argument parser made things simpler, but...maybe it doesn't? Hopefully, this is a thing we can keep trying to improve.

@schneems schneems enabled auto-merge (squash) June 24, 2024 15:10
From https://www.reddit.com/r/rust/comments/1dkhl0n/my_rust_error_handling_best_practices/ (full text below). Which links to this video https://www.youtube.com/watch?v=j-VQCYP7wyw&t=1s. The overall goal of this commit is to reduce complexity and code.

We can remove the "full" error handling i.e. wrapping every error in a custom enum because this is a "script" where "ease of editing" and "speed to read" are more important than long term extensibility and error-correctness. Instead we can use `Box<dyn std::error::Error>`. 

We can choose to "upgrade" error handling later using the progressive steps listed below:

## Text from linked url

```
Here are my error best practices I have matured to. This is working pretty well for me.

5 points summary: (but watch the video for the full context and examples)

For tests & examples, Box<dyn Error>. Zero dependency, simple, almost as flexible as Anyhow, but best of all "progressive" with the production way to do the ? and error handling.

Next for production code, I have Error as enum, with a Custom(String) variant, with the from &str and from String, and I start to build other variants.

As the code matures, I remove the Error::Custom(String) and make sure all is fully typed.

Also, very important, I make sure the WHOLE meaning is captured in the variant name and properties name. (The display is mostly for command line utilities.)

I like to use derive_more rather than thiserror to remove the From/Display boilerplate. It allows me to use the From, and wire the debug as the display for non-command line errors (as they will be captured in a structured way for further rendering anyway).

The best of this approach is that it is very progressive from 1...5, and I do not expose any "lib" way to handle Error, as all of the Errors are standard Rust Errors.

There is no absolute truth, but just wanted to share what is working well for me.
```
@schneems
Copy link
Contributor Author

We talked offline about:

  • Reducing the error handling logic using anyhow or Box<dyn std::error::Error> (goal is less code)
  • Exploring using a rust nightly feature that allows inline cargo toml declaraions for scripting
#!/usr/bin/env -S cargo +nightly -Zscript
/// ```cargo
/// [package]
/// edition = "2021"
/// ```
fn main() {
  // script goes here!
}

@schneems
Copy link
Contributor Author

Turns out this is the correct syntax for being able to download dependencies:

#!/usr/bin/env -S cargo +nightly -Zscript
---
[package]
edition = "2021"

[dependencies]
java-properties = "2.0.0"
glob = "0.3.1"
clap = { version = "4.5.4", features = ["derive"] }
shared = { path = "/Users/rschneeman/Documents/projects/work/docker-heroku-ruby-builder/shared" }
// ...
---

I got it working, but I don't think we should move forward here:

image

That means that cargo test will pass even if bin/ruby_check.rs has a syntax error, which is not a great development experience.

I also had to move away from <dep-name> = { workspace = true } to be able to manage dependencies as shared due to this constraing.

  • The syntax or features may change, based on how I thought it should work. That could mean that running rustup update could break this deploy script, and we would have to stop and fix it. We could work around that by pinning to a specific rustup version, similar to how I'm doing it inside of docker, but I wanted to document it as a downside either way.

I'm pushing my code to a branch for reference to possibly share this feedback experience where someone can also view the code https://github.com/heroku/docker-heroku-ruby-builder/tree/schneems/riir-rust-script-nightly-experiment-jun-26.

Copy link
Contributor

@colincasey colincasey left a comment

Choose a reason for hiding this comment

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

In general I think this approach of using Rust for external script needs and Bash for internal container needs is a reasonable trade-off.

I'd like to see the Rust parts be more script-ish which, I think, would simplify the project in general, some of those features are not fully baked yet. Using cargo run --bin ... and requiring the full cargo workspace crate layout is less than ideal but it's fine for now.

.github/workflows/build_jruby.yml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
README.md Show resolved Hide resolved
ruby_executable/src/lib.rs Show resolved Hide resolved
@schneems schneems merged commit b54075e into main Jul 2, 2024
22 checks passed
@schneems schneems deleted the schneems/riir-hybrid branch July 2, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants