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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Godot version check + workaround for API generation bug #838

Merged
merged 2 commits into from Jan 8, 2022

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Jan 1, 2022

As mentioned in #833, Godot versions < 3.3.1 were subject to a bug that caused non-deterministic crashes during the command:

godot --gdnative-generate-json-api api.json

For users working with affected Godot versions (e.g. 3.2), this makes the feature flag custom-godot annoying to use, since they can't rely on the API generation to succeed -- let alone use it for automation/CI. This PR works around that by retrying the command up to 10 times (magic number). I changed the minimum supported Godot version in our own CI again to 3.2, meaning that if it doesn't work, we have to suffer, too 馃槵

Additionally, this PR parses the Godot version and emits a meaningful error message if an unsupported version is detected (3.1 or 4.0).

@Bromeon Bromeon added c: tools Component: tooling, tests, IDEs, Cargo, Rust ecosystem c: ci Component: CI and automation quality-of-life No new functionality, but improves ergonomics/internals labels Jan 1, 2022
@Bromeon Bromeon added this to the v0.10.0 milestone Jan 1, 2022
@Bromeon Bromeon force-pushed the qol/godot-version branch 2 times, most recently from 4304344 to b9ac0ac Compare January 1, 2022 12:17
@Bromeon
Copy link
Member Author

Bromeon commented Jan 1, 2022

Where I was unsure: the whole version logic is in a separate file, to prevent build.rs from growing too much. The problem is, there seems to be no agreed-upon best practice regarding build script separation.

Current approach is this (in build.rs):

#[cfg(feature = "custom-godot")]
mod godot_version {
    include!("src/godot_version.rs");
}

I also tried the following, but my build step got stuck in an endless loop:

#[path = "src/godot_version.rs"]
mod godot_version;

Ideas?

Would also be nice if the tests in godot_version.rs would be run during cargo test. I don't want the version logic to be part of the compiled gdnative-bindings crate, so adding the module in lib.rs is not really an option...

bors try

bors bot added a commit that referenced this pull request Jan 1, 2022
@bors
Copy link
Contributor

bors bot commented Jan 1, 2022

try

Build succeeded!

And happy new year! 馃帀

@chitoyuu
Copy link
Contributor

chitoyuu commented Jan 1, 2022

Where I was unsure: the whole version logic is in a separate file, to prevent build.rs from growing too much. The problem is, there seems to be no agreed-upon best practice regarding build script separation.

Perhaps the logic can be put into bindings_generator? It's true that version parsing isn't directly related to code generation, but it is the crate that contains "the rest of build.rs".

@Bromeon
Copy link
Member Author

Bromeon commented Jan 2, 2022

@chitoyuu Great idea, I moved it to bindings_generator.

I also set up a mass CI test for all 3.x versions: https://github.com/Bromeon/godot-rust/actions/runs/1646049051
This shows two things:

  • it rejects versions < 3.2 with a helpful error message
  • it works for versions >= 3.2 and < 3.3.1 on first attempt

Godot 4 is not available for download, but I tested it locally, also rejected.

@Bromeon
Copy link
Member Author

Bromeon commented Jan 8, 2022

bors r+

bors bot added a commit that referenced this pull request Jan 8, 2022
838: Godot version check + workaround for API generation bug r=Bromeon a=Bromeon

As mentioned in #833, Godot versions < 3.3.1 were subject to [a bug](godotengine/godot#48081) that caused non-deterministic crashes during the command:
```
godot --gdnative-generate-json-api api.json
```

For users working with affected Godot versions (e.g. 3.2), this makes the feature flag `custom-godot` annoying to use, since they can't rely on the API generation to succeed -- let alone use it for automation/CI. This PR works around that by retrying the command up to 10 times (magic number). I changed the minimum supported Godot version in our own CI again to 3.2, meaning that if it _doesn't_ work, we have to suffer, too 馃槵

Additionally, this PR parses the Godot version and emits a meaningful error message if an unsupported version is detected (3.1 or 4.0).

Co-authored-by: Jan Haller <bromeon@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jan 8, 2022

Build failed:

Move logic from build.rs to bindings_generator
@Bromeon
Copy link
Member Author

Bromeon commented Jan 8, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 8, 2022

Build succeeded:

@bors bors bot merged commit b2a9832 into godot-rust:master Jan 8, 2022
@Bromeon Bromeon deleted the qol/godot-version branch January 8, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: ci Component: CI and automation c: tools Component: tooling, tests, IDEs, Cargo, Rust ecosystem quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants