Skip to content

[Discussion] Simplify custom bindings generation for custom builds #640

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

Closed
rminderhoud opened this issue Dec 3, 2020 · 6 comments · Fixed by #833
Closed

[Discussion] Simplify custom bindings generation for custom builds #640

rminderhoud opened this issue Dec 3, 2020 · 6 comments · Fixed by #833
Labels
c: bindings Component: GDNative bindings (mod api) feature Adds functionality to the library good first issue
Milestone

Comments

@rminderhoud
Copy link

Hello, I just wanted to make a small proposal to this wonderful crate. Namely, to improve the ergonomics for when using a custom build of godot.

Problem
The problem is as such, a user wants to use godot-rust with a custom build of godot so they need a custom build of gdnative-bindings to use godot-rust. The current process is nicely documented here but it is quite convoluted. To summarize, when using a custom godot build we need to rebuild the gdnative-bindings with the correct api.json exported from our custom godot build. The recommended steps are to download the gdnative-bindings crate, modify the stored api.json file, and then do some magic in our project's Cargo.toml.

Criticism
My main argument against this implementation is that it's neither here, nor there. On the one hand, there's the kind of user that is comfortable customizing godot, making a custom build, using these bindings, etc. For those people I think we can safely assume that they would be comfortable maintaining a fork of this repo and can simply change the api.json directly. For these power users, I would argue the guidance can be simplified to "regenerate the api.json here and rebuild".

On the other hand I think there is a second type of user who uses a custom build of godot but maybe makes very minor modifications (disable specific modules, compiler tweaks, etc.) or none at all (e.g. is on a minor version beta). For this user I think the steps outlined in the docs are too cumbersome. If I am on 3.2.3-stable but I want to test my project on 3.2.4-beta then there's quite a bit of ceremony to update my gdnative lib.

Proposal
I believe we can simplify the process by leveraging the build file in gdnative-bindings. I propose that using a feature flag and an environment variable, the api.json can be automatically generated at build time. This would allow users to use the latest sources without having to modify any source code but still be able to target a custom build. Here is an example of how I would foresee this working.

A new feature flag would be introduced in the top level crate that gets forwarded to the gdnative-bindings crate. Users who want to use a custom build would specify this feature flag.

[dependencies.gdnative]
version = "0.9.1"
features = ["godot_custom_build"]

The user would then also have to specify an environment variable to their custom godot build, e.g. GODOT_BIN. The feature could then be detected in gdnative-bindings/build.rs and if it is enabled and the var set then the api.json can be re-built on the fly.

if cfg!(feature = "godot_custom_build") {
    let godot_bin = env::var("GODOT_BIN").expect("GODOT_BIN not set");

    // Call std::process and generate the api.json
}

I believe this would be a big enhancement to support the workflow of custom builds and would not be too disruptive. It would require adding a feature flag to the crates, adding the feature check to the build.rs and adding some minor logic to pick the correct api.json after it has been generated.

@ghost ghost added c: bindings Component: GDNative bindings (mod api) feature Adds functionality to the library labels Dec 9, 2020
@ghost
Copy link

ghost commented Dec 9, 2020

Seems like a good idea to me. Thanks for the suggestion! Could you create a PR for this?

@ghost ghost added the good first issue label Jan 11, 2021
@carmel4a
Copy link

carmel4a commented Jul 12, 2021

Is the feature flag necessary? It may work that way: if some env var (let's say GODOT_BIN) is present and not empty - this behavior will occur, otherwise default api.json will be used. User will have even less to do to use custom Godot.
If nobody works on it, I may try to do this in +/- 2 weeks.

@bluenote10
Copy link
Contributor

The user would then also have to specify an environment variable to their custom godot build, e.g. GODOT_BIN.

Could we also use which::which("godot") and fallback to $GODOT_BIN if it isn't set? I'd assume that Linux users typically symlink the binary into their path anyway.

bors bot added a commit that referenced this issue Dec 15, 2021
829: Support Godot 3.4 r=Bromeon a=Bromeon

Updates api.json, CI scripts and examples.

This is a **breaking change**. Some GDNative APIs added default parameters, which can currently not be modeled in a backwards-compatible way in godot-rust (Rust itself doesn't support default parameters). It is however possible to manually generate `api.json` for an older Godot version (3.2 or 3.3) and keep using godot-rust normally.

Addresses #814 in the short-term 🛠️ 
Still on the radar: #640 📡 

Co-authored-by: Jan Haller <[email protected]>
bors bot added a commit that referenced this issue Dec 15, 2021
829: Support Godot 3.4 r=Bromeon a=Bromeon

Updates api.json, CI scripts and examples.

This is a **breaking change**. Some GDNative APIs added default parameters, which can currently not be modeled in a backwards-compatible way in godot-rust (Rust itself doesn't support default parameters). It is however possible to manually generate `api.json` for an older Godot version (3.2 or 3.3) and keep using godot-rust normally.

Addresses #814 in the short-term 🛠️ 
Still on the radar: #640 📡 

Co-authored-by: Jan Haller <[email protected]>
@Bromeon
Copy link
Member

Bromeon commented Dec 20, 2021

@rminderhoud and @bluenote10: Thanks a lot for your input! I realized your ideas as a proof-of-concept in #833.

Let me know if you have time to test it or other feedback (you could leave it directly in the PR)! 🙂

@carmel4a: I think it's important that godot-rust in its default configuration behaves consistently. If the presence of an executable on the system alters runtime behavior (to the point of incompatibility) without the user opting in, that will lead to unpleasant surprises. That's why custom-godot is now a default-off feature flag.

@Bromeon Bromeon modified the milestones: v0.10.1, v0.10.0 Dec 21, 2021
@bors bors bot closed this as completed in 66b2dd7 Jan 1, 2022
@rminderhoud
Copy link
Author

@Bromeon, happy new year. I didn't get a chance to test this before it merged but the changes look awesome!

@Bromeon
Copy link
Member

Bromeon commented Jan 2, 2022

@rminderhoud Thanks, happy new year to you too! 🍾

Glad to hear! No worries about the testing, we can continuously enhance the experience. Now that it's in master, many more people have the chance to try it out and give feedback. There's already a follow-up PR that improves robustness: #838

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bindings Component: GDNative bindings (mod api) feature Adds functionality to the library good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants