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

compile: Upgrade to bindgen 0.61 and use Builder with Clone #22

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

MarijnS95
Copy link
Contributor

Depends on #21 for pub use bindgen; (perhaps all extern crate should be replaced since moving to edition 2021?)

As announced earlier in #20 when introducing BindgenOptions, upstream bindgen didn't yet allow cloning nor copying their Builder struct, limiting it to generate a single binding per Builder object. This doesn't fit with ispc-rs's Config struct which explicitly allows multiple libraries to be compiled using (partially) the same base configuration (barring any side-effects from fn compile(&mut self, lib) updating various fields depending on lib).

This issue has since been addressed upstream, and now allows us to move and clone the bindgen::Builder around instead of implementing a very limited subset of config fields and manually reconstructing the builder every time.

Commit ad85f13 ("Seems like edition in workspace isn't applied? Update
all to 2021") found that `workspace.edition` isn't a valid key and added
it to most crates' individual `[package]` table, but didn't remove the
key from `[workspace]` nor added it to the `compile` and `runtime`
crates.

Note that `[workspace.package] edition = "2021"` exists since Rust 1.64,
but still requires `[package] edition.workspace = true` to be repeated
in every individual `Cargo.toml` for it to be inherited:
https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table
As announced earlier in Twinklebear#20 when introducing `BindgenOptions`, upstream
`bindgen` didn't yet allow cloning nor copying their `Builder` struct,
limiting it to generate a single binding per `Builder` object.  This
doesn't fit with ispc-rs's `Config` struct which explicitly allows
multiple libraries to be compiled using (partially) the same
base configuration (barring any side-effects from `fn compile(&mut self,
lib)` updating various fields depending on `lib`).

This issue has since been addressed upstream, and now allows us to move
and clone the `bindgen::Builder` around instead of implementing a very
limited subset of config fields and manually reconstructing the builder
every time.
@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Oct 20, 2022

In hindsight I'm not really sure if it was/is a good idea to expose the entire bindgen Builder struct (EDIT: In #20 (review) it was pointed out that it'd be nice to expose all config options to the user, I suppose to give them maximum control for i.e. experimenting), since it's pretty tied to ispc what bindings end up being generated from the header, and what config options ultimately make sense / should be settable by the user.

At the same time updating this bindgen_builder struct means reconstructing a new bindgen::Builder and passing it to bindgen_builder() (but most builder functions are additive anyways, iirc, so it might be better to "start over" in most cases or let the user figure out when to .clone() an earlier copy of their builder). It might be desired to "move" a builder out of and back into Config though, within a closure or just by .take()-replacing it with Default.

(barring any side-effects from fn compile(&mut self, lib) updating various fields depending on lib)

Perhaps we should refactor this.

@Twinklebear
Copy link
Owner

Thanks @MarijnS95 ! I think this will work well to expose the full bindgen options so that people can control whatever bindgen settings they want to without having to request each one to be added to ispc-rs. The best way to build things/clone/reuse/etc will depend on what different app's build configurations are like so just exposing the options is a good path.

Good point about the side effects in compile, that's something that would be good to address too so that the Config is more re-usable after being setup.

@Twinklebear Twinklebear merged commit 67c26dc into Twinklebear:master Oct 25, 2022
@Twinklebear
Copy link
Owner

I'm not sure if we'd want to make the additional fixes to make compile not modify things (so it can be more re-used) and then put out a release or if it'd be helpful to push this up sooner as a release? The changes here break the API for the bindgen setting so it'd be a major version bump

@MarijnS95 MarijnS95 deleted the bindgen-builder-clone branch October 25, 2022 12:06
@MarijnS95
Copy link
Contributor Author

@Twinklebear Perhaps a good idea to fold in those changes at the same time, assuming they may be breaking as well?

@Twinklebear
Copy link
Owner

Yeah, that's my thought as well. I should have some time to make those changes this week/weekend

@MarijnS95
Copy link
Contributor Author

@Twinklebear Thanks! Better if you look into it 👍

@Twinklebear
Copy link
Owner

@MarijnS95 just to give you a heads up, I made that last change to the Config::compile API and pushed 2.0 out: https://github.com/Twinklebear/ispc-rs/releases/tag/2.0.0

MarijnS95 added a commit to Traverse-Research/intel-tex-rs-2 that referenced this pull request Nov 7, 2022
We landed some nice changes to smooth out `bindgen::Builder` usage with
`bindgen 0.61` 🎉: Twinklebear/ispc-rs#22
@MarijnS95
Copy link
Contributor Author

@Twinklebear thanks a ton! I've just upgraded our crates to use the new release and it all seems to work great! Immutability changes look great too!

MarijnS95 added a commit to Traverse-Research/intel-tex-rs-2 that referenced this pull request Feb 28, 2023
We landed some nice changes to smooth out `bindgen::Builder` usage with
`bindgen 0.61` 🎉: Twinklebear/ispc-rs#22
MarijnS95 added a commit to Traverse-Research/intel-tex-rs-2 that referenced this pull request Feb 28, 2023
* Upgrade `ispc-rs` to `2.0.0`

We landed some nice changes to smooth out `bindgen::Builder` usage with
`bindgen 0.61` 🎉: Twinklebear/ispc-rs#22

* Fix stricter `clippy::needless_borrow` lint and clean up some existing code

* Clean old artifacts

* CI/generate-binaries: `{}` glob pattern not supported in upload-artifact

https://github.com/actions/toolkit/tree/main/packages/glob#patterns

* Update binaries from CI
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.

None yet

2 participants