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

Refactor the inline Python commands into a Python binary. #180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xinhaoyuan
Copy link
Collaborator

This is needed when python3 does not exist in the runtime environment.

@fmeum
Copy link
Member

fmeum commented Oct 6, 2021

Don't worry about the Jazzer launcher script. I am
currently working on completely redoing the logic for how Jazzer finds the JVM. As a side effect of this effort, it will no longer need Python.

This is needed when `python3` does not exist in the runtime environment.
Copy link
Collaborator

@stefanbucur stefanbucur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for simplifying the launcher scripts, and sorry for the delay.

I am wondering if there is a clean way to eliminate the (new) duplication in the launcher scripts, too, since I expect every launcher script would need to add this data dependency.

Could we somehow make this tool available as a data dependency on the action that invokes the launcher? This way, we could augment the script calling the launcher to extend the $PATH with the location of the tool, so the script can directly use $(realpath ${FUZZER_BINARY}).

@stefanbucur
Copy link
Collaborator

Thanks for simplifying the launcher scripts, and sorry for the delay.

I am wondering if there is a clean way to eliminate the (new) duplication in the launcher scripts, too, since I expect every launcher script would need to add this data dependency.

Could we somehow make this tool available as a data dependency on the action that invokes the launcher? This way, we could augment the script calling the launcher to extend the $PATH with the location of the tool, so the script can directly use $(realpath ${FUZZER_BINARY}).

I've just realize we might have another alternative: Guarantee that $FUZZER_BINARY is an absolute path, and make sure the places that set it do so correctly. For the fuzzing launcher, this is conveniently Python code so we can inline the contents of realpath there: https://github.com/bazelbuild/rules_fuzzing/blob/4bafba51ffd9d418d236adb61de36fda1a90e764/fuzzing/tools/launcher.py#L95

There is one more place which is currently Bash, but I'm wondering if that rule is actually still needed since we have the more general launcher rule: https://github.com/bazelbuild/rules_fuzzing/blob/4bafba51ffd9d418d236adb61de36fda1a90e764/fuzzing/private/regression.bzl#L25

@xinhaoyuan
Copy link
Collaborator Author

I tried to put the tool dependency inside the launcher rule, but I didn't like it since it makes the testing hard (matching the runfiles in engine_test.bzl). Now thinking of it, I think not all engines require realpath (e.g. honggfuzz), so it might be nicer to have each engine decide to depend on the tool or not.

If canonical path is not required, we can rewrite the bash script, e.g. append ./ to the path if it does not start with /. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants