test: support switching API versions in check.sh

[adalinesimonian]

Allows easily testing against a given version of the API bindings simply by passing a version to the -a/--api-version parameter.

I've been using this patch on my machine for integration testing.

[lilizoey]

do we have to add support for specifically gdvm here? isn't the main advantage of this PR that you can specify a godot api version to compile against? i feel like it should be possible to do something like GODOT_BIN="gdvm run 4.2" ./check.sh itest -g 4.2 if you wanted to use gdvm specifically

[adalinesimonian]

For the API features to get built in line with the version it felt cleaner to just roll it all together rather than chain two commands together. But I'm not picky if you have a preferred alternative.

[lilizoey]

i do agree that it's cleaner, but only if you are already using gdvm. so id prefer changing it so that it only sets the api version feature.

also since almost everything is related to godot, including "godot" in --godot-version feels a bit redundant now. maybe -a/--api-version or just -v/--version? especially since with this change, we're only changing the api version we compile against, and not the binary used to run the program. which is also a benefit of splitting this feature up, as we do try to ensure that we can run in backwards compatible configurations (like compiling against 4.2 but running with 4.3).

[adalinesimonian]

Those are great points. Let me see if the bin path var would let arguments right now the way it is. I think possibly it would need an addl. var for arguments based on how it is being included in the script (e.g. GODOT4_BIN="gdvm" GODOT4_ARGS="run --console 4.2 --force" ./check.sh itest -v 4.2 which is quite a mouthful). One option is also to have a --gdvm flag which would generate the correct command automatically, at which point you just run ./check.sh itest --gdvm [optional version] -v 4.2 or something along those lines. gdvm does expose a godot binary to the path, but since the version is not pinned or set in the project, it will try to run the version for the project file, which seems to be 4.1. Using the gdvm command on the terminal is the only way to specify a version at runtime on the CLI.

[adalinesimonian]

I just split the API version from the version mechanism for gdvm and updated this PR's description. This allows for a clean and simple syntax with gdvm installed but allows selecting the API version if not using gdvm, also. What're your thoughts?

[Bromeon]

Thank you for your contribution!

While I appreciate the integration efforts with your project gdvm (I assume this one), I don't think it's in godot-rust's scope to integrate to third-party tools, unless they're official or so widely used that they've become de-facto standard in the Godot ecosystem. There are various reasons for this: unclear which tools should be integrated, who maintains the integration, if/how they are updated, whether their API is stable, more testing/bug surface and overall complexity/scope of the project. This reasoning also extends to tooling such as the check.sh script.

If you're interested in customizing your workflow, our .gitignore features a /script entry, in which you can add various scripts without interfering with files under version control. Regarding CI, we already test multiple versions, with various feature flags. If you fork the project, you can use GitHub Actions in your own repository and push changes there until they're ready for a PR. I find the feedback times quite reasonable (5-8min for full CI, 2-4 for minimal CI).

Apart from that, check.sh is mostly meant as a quick local check, but the CI workflows remain the authority on what's considered correct/suitable (also in case of discrepancies between CI and check.sh). Considering that, the -a|--api-version to set the api-version feature seems like a good addition, but functionality beyond that should probably be delegated to third-party tools.

So it would be nice if you could reduce the PR to the -a|--api-version flag. I feel a bit bad for the effort that went into the other parts, but please don't hesitate to discuss ideas before implementation next time 🙂

[adalinesimonian]

I can split off the -g flag into my own script. Don't worry about any effort wasted, it really wasn't. I made this change primarily for myself when I was testing against API versions and opened up this PR in the event it might be helpful to others. Since this script is just for the convenience of contributors, I didn't really think it be the same as an integration into the actual project. I'll move the sugar into a script in script/.

I'll add a GODOT_ARGS parameter so that arguments can be passed to godot before any are added by this script. That would make the full example of usage all together:

# Windows:

# Test against specifically 4.3.0 Godot with API version 4-3
GODOT4_BIN="gdvm" GODOT_ARGS="run --force --console 4.3.0 --" ./check.sh itest -a 4.3

# Test against latest 4.3.x Godot with API version 4-3
GODOT4_BIN="gdvm" GODOT_ARGS="run --force --console 4.3 --" ./check.sh itest -a 4.3

# Other platforms (*nix, macOS):

# Test against specifically 4.3.0 Godot with API version 4-3
GODOT4_BIN="gdvm" GODOT_ARGS="run --force 4.3.0 --" ./check.sh itest -a 4.3

# Test against latest 4.3.x Godot with API version 4-3
GODOT4_BIN="gdvm" GODOT_ARGS="run --force 4.3 --" ./check.sh itest -a 4.3

Which is a bit long and clunky, but it works.

I'll get to this when I have a moment.

[Bromeon]

Would something like this not work?

GODOT4_BIN="gdvm run --console 4.3 --"

[adalinesimonian]

Not the way it's being used in the script, no:

run "$godotBin" --path itest/godot --headless -- "[${extraArgs[@]}]"

This would give you a gdvm run 4.3 --: command not found since it's not being expanded. but expanding the variable might cause issues if anyone's been passing GODOT4_BIN to check.sh with spaces in the path.

[adalinesimonian]

Addressed comments and rebased onto main.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1016

[Bromeon]

Thanks for the updates! 🙂

I tested this locally. One problem is that for other API versions than the default, there may be unused symbols and other warnings. At the moment, clippy is strict in the sense that any warnings are treated as errors.

Example: if I run

./check.sh -a 4.1

---

I don't really feel like adding thousand #[cfg] to "fix" all these occurrences, it adds lots of busywork and hard-to-read code for little value. We don't test CI clippy against all possible features, so those will keep creeping back in, anyway.

We could maybe pass -D warnings to cargo build calls in CI, but I'd say that's another discussion outside the scope of these changes.

Either way, check.sh failing on master will be treated as a bug by users, so we need to address this. Maybe simplest is to skip clippy invocations as soon as -a is provided (even if it's the current one), and instead print a warning/note and succeed. What do you think?

Would also need a rebase onto master.

[adalinesimonian]

Either way, check.sh failing on master will be treated as a bug by users, so we need to address this. Maybe simplest is to skip clippy invocations as soon as -a is provided (even if it's the current one), and instead print a warning/note and succeed. What do you think?

I think that's a good idea. If clippy is being manually supplied I think it should still run it with a warning, and only omit it out of the checks run by default. If that works I can go ahead and make the change.

[adalinesimonian]

[Bromeon]

Thanks a lot, looks great now!