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

Remove rz_test_chdir() #4766

Merged
merged 1 commit into from
Dec 14, 2024
Merged

Remove rz_test_chdir() #4766

merged 1 commit into from
Dec 14, 2024

Conversation

kazarmy
Copy link
Member

@kazarmy kazarmy commented Dec 14, 2024

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

I've finally realized that the likely reason for the binrz/rz-test/ requirement in rz_test_chdir() is that it assumes an in-source (or in-tree) build, where binaries are placed next to their source when building. Meson only allows out-of-source (or out-of-tree) builds, and additionally we have a meson install step, so the final location of Rizin binaries can't really be used to find the Rizin test dirs.

Thus, this pr removes the not-relevant-to-us rz_test_chdir() code, with the extra benefit that adding ./ to a test filename doesn't result in surprising behavior (as noted in #4759). I've added extra details to the rz_test_chdir_fromtest() Doxygen comment since the code can fall back to the current working directory if a test filename isn't provided i.e. rz-test is invoked without an argument.

Test plan

The above makes sense and there are no significant regressions like what is described in #4759. All builds are green.

Closing issues

...

@kazarmy
Copy link
Member Author

kazarmy commented Dec 14, 2024

Or maybe this should be kept? One can possibly link to the binaries in the Meson build dir and so this could be useful. Perhaps I should just fix rz_test_chdir() and update the Doxygen comment.

@kazarmy kazarmy closed this Dec 14, 2024
@kazarmy
Copy link
Member Author

kazarmy commented Dec 14, 2024

Perhaps best to simplify matters for now and add search from binary location later

@kazarmy kazarmy reopened this Dec 14, 2024
@kazarmy
Copy link
Member Author

kazarmy commented Dec 14, 2024

... maybe add search from binary location later

@kazarmy kazarmy merged commit 7fa052d into rizinorg:dev Dec 14, 2024
88 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants