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

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

Closed
Bogay opened this issue Oct 18, 2022 · 5 comments · Fixed by #966
Closed

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

Bogay opened this issue Oct 18, 2022 · 5 comments · Fixed by #966
Labels
bug c: tools Component: tooling, tests, IDEs, Cargo, Rust ecosystem
Milestone

Comments

@Bogay
Copy link
Contributor

Bogay commented Oct 18, 2022

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.

@Bogay Bogay added the bug label Oct 18, 2022
@Bromeon Bromeon added the c: tools Component: tooling, tests, IDEs, Cargo, Rust ecosystem label Oct 18, 2022
@Bromeon
Copy link
Member

Bromeon commented Oct 18, 2022

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
Copy link
Contributor Author

Bogay commented Oct 18, 2022

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
Copy link
Member

Bromeon commented Oct 18, 2022

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?

@Bromeon Bromeon added this to the v0.11.x milestone Oct 18, 2022
@Bogay
Copy link
Contributor Author

Bogay commented Oct 18, 2022

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
Copy link
Contributor Author

Bogay commented Oct 18, 2022

Would you like to open a PR that fixes both things? 🙂

opened 👍

bors bot added a commit that referenced this issue Oct 18, 2022
966: fix(check.sh): make it more portable r=Bromeon a=Bogay

adjust the pattern to match output lib path, and add shebang to ensure which shell to use.

Fixs #965 

Co-authored-by: bogay <[email protected]>
@bors bors bot closed this as completed in #966 Oct 18, 2022
@chitoyuu chitoyuu modified the milestones: v0.11.x, v0.11.1 Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: tools Component: tooling, tests, IDEs, Cargo, Rust ecosystem
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants