godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.61k stars 210 forks source link

Path of `gdnative-test` lib is different from the one specify in `check.sh` #965

Closed Bogay closed 1 year ago

Bogay commented 1 year ago

In check.sh, it copies target/debug/gdnative_test* to test/project/lib and then run integration tests.

    itest)
        findGodot
        cmds+=("cargo build --manifest-path test/Cargo.toml --features $features")
        cmds+=("cp target/debug/gdnative_test* test/project/lib/")
        cmds+=("$godotBin --path test/project")
        ;;

https://github.com/godot-rust/godot-rust/blob/master/check.sh#L77-L82

But the path of gdnative-test is target/debug/ilbgdnative_test* on my computer. (Ubuntu 20.04 on WSL)

❯ ll target/debug 
total 66M
drwxr-xr-x  66 bogay bogay 4.0K Oct 14 04:28 build
drwxr-xr-x   5 bogay bogay 168K Oct 14 04:29 deps
drwxr-xr-x   2 bogay bogay 4.0K Oct 14 03:39 examples
drwxr-xr-x 106 bogay bogay 4.0K Oct 14 04:29 incremental
-rw-r--r--   1 bogay bogay 8.4K Oct 14 04:27 libgdnative_test.d
-rwxr-xr-x   2 bogay bogay  65M Oct 14 04:28 libgdnative_test.so

Should we change the command to cp target/debug/*gdnative_test* test/project/lib/ or there is anyway we can find where the output lib goes?

BTW, is it reasonable to use some pre-commit tools like rusty-hook to maintain these checks? I usually use pre-commit in my python projects and husky for js project, not sure whether any popular alternative for rust project.

Bromeon commented 1 year ago

Thanks for the report! If you replace

        cmds+=("cp target/debug/gdnative_test* test/project/lib/")

with

        cmds+=("cp target/debug/*gdnative_test* test/project/lib/")

does it work?

We had a pre-commit hook in the past, I'm not a big fan of them to be honest. It's unnecessarily intrusive and can slow down the workflow significantly. We have CI that already checks everything and allows for async workflow (while my previous commit is checking, I can already work on the next thing).

The check.sh's purpose is mainly to provide an alternative to CI, when you're offline or want to run it locally on your faster machine. It's a best-effort script for convenience, but CI is the authority when it comes to correctness πŸ™‚

Bogay commented 1 year ago

Thanks for the report! If you replace

        cmds+=("cp target/debug/gdnative_test* test/project/lib/")

with

        cmds+=("cp target/debug/*gdnative_test* test/project/lib/")

does it work?

Yeah, it works.

BTW, this script is not compatible with zsh (and fish). So I suggest that we should add a shebang for it to ensure it's executed by bash. (if execute it directly, i.e. ./check.sh)

Bromeon commented 1 year ago

Would you like to open a PR that fixes both things? πŸ™‚

if execute it directly, i.e. ./check.sh

Does it have the right chmod permissions for that at the moment?

Bogay commented 1 year ago

Would you like to open a PR that fixes both things? πŸ™‚

if execute it directly, i.e. ./check.sh

Does it have the right chmod permissions for that at the moment?

Yes. The error message looks like this:

$ echo $SHELL
/usr/bin/zsh

$ ll check.sh
-rwxr-xr-x 1 bogay bogay 3.2K Oct 19 00:58 check.sh

$ ./check.sh
./check.sh: 7: Syntax error: "(" unexpected (expecting "fi")
Bogay commented 1 year ago

Would you like to open a PR that fixes both things? πŸ™‚

opened πŸ‘