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

Generate mruby bindings locally and check them in to the repository instead of during build time #2514

Merged
merged 7 commits into from
Apr 24, 2023

Conversation

lopopolo
Copy link
Member

Check in the outputs of running bindgen on mruby and the associated C extensions required for building artichoke-backend.

bindgen is a heavy dependency with many transitives that don't already show up in Artichoke's Cargo.lock file (e.g. clang-sys). This was the motivation for replacing a Rust dev-dependency on bindgen with a requirement that the bindgen CLI be available ambiently in PATH when building Artichoke:

Because bindgen is available by default in GitHub Actions runners and this requirement was able to be documented in CONTRIBUTING.md, this seemed like an acceptable loss of build hermeticity. Additionally, relying on pre-built bindgen sped up clean build compilation time significantly.

The maintenance burden of this approach has been increasing recently as the bindgen CLI interface has undergone more rapid iteration:

Instead, add a bindgen Rake task (invokable as bundle exec rake bindgen) which uses the bindgen CLI on a developer laptop to generate the bindings once and dumps them into the source tree to be checked in.

This allows removing a bunch of code in artichoke-backend's build.rs and makes the code authorship experience better by improving browsing of generated bindings, ensuring goto definition works, and reducing build times.

The reduction in runtime of artichoke-backend's build.rs means the long poles for Artichoke compilation are tzdb and artichoke-backend (both of these crates have a huge amount of code):

Screenshot 2023-04-24 at 1 48 33 PM

@lopopolo lopopolo added A-build Area: CI build infrastructure. A-vm Area: Interpreter VM implementations. A-performance Area: Performance improvements and optimizations. A-deps Area: Source and library dependencies. B-mruby Backend: Implementation of artichoke-core using mruby. labels Apr 24, 2023
@lopopolo
Copy link
Member Author

markdown link check job failure is a flake on dependabot.com

@lopopolo lopopolo merged commit b5ea8ae into trunk Apr 24, 2023
@lopopolo lopopolo deleted the lopopolo/artichoke-backend-sys-check-in-bindgen branch April 24, 2023 21:13
@lopopolo
Copy link
Member Author

Related change to remove bindgen from Docker nightly build containers is here:

lopopolo added a commit to artichoke/docker-artichoke-nightly that referenced this pull request Apr 25, 2023
@lopopolo
Copy link
Member Author

playground is busted now due to these changes:

https://github.com/artichoke/playground/actions/runs/4792255663/jobs/8523565662

--- stderr
  error: unexpected argument '--size_t-is-usize' found

    tip: a similar argument exists: '--no-size_t-is-usize'

  Usage: bindgen [FLAGS] [OPTIONS] [HEADER] -- [CLANG_ARGS]...

  For more information, try '--help'.
  thread '<unnamed>' panicked at 'bindgen failed', /home/runner/.cargo/git/checkouts/artichoke-c12ffc01676ba375/fbbe649/artichoke-backend/build.rs:315:9
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
     1: core::panicking::panic_fmt
               at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
     2: build_script_build::libs::bindgen
               at ./build.rs:315:9
     3: build_script_build::libs::build::{{closure}}::{{closure}}
               at ./build.rs:328:17
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
  thread 'main' panicked at 'a scoped thread panicked', /home/runner/.cargo/git/checkouts/artichoke-c12ffc01676ba375/fbbe649/artichoke-backend/build.rs:319:9
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
     1: core::panicking::panic_fmt
               at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
     2: std::thread::scoped::scope
               at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/thread/scoped.rs:157:13
     3: build_script_build::libs::build
               at ./build.rs:319:9
     4: build_script_build::main
               at ./build.rs:361:5
     5: core::ops::function::FnOnce::call_once
               at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@lopopolo
Copy link
Member Author

this busted artichoke builds on 32 bit architectures, in particular wasm32-unknown-emscripten.

lopopolo added a commit that referenced this pull request Apr 25, 2023
…ckend-sys-check-in-bindgen"

This reverts commit b5ea8ae, reversing
changes made to 39e377f.

This led to regressions with building Artichoke on the playground for
wasm32-unknown-emscripten (and presumably other 32-bit targets).
lopopolo added a commit that referenced this pull request Apr 25, 2023
Revert #2514, run bindgen at build time with a `bindgen` build dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build Area: CI build infrastructure. A-deps Area: Source and library dependencies. A-performance Area: Performance improvements and optimizations. A-vm Area: Interpreter VM implementations. B-mruby Backend: Implementation of artichoke-core using mruby.
Development

Successfully merging this pull request may close these issues.

1 participant