-
Notifications
You must be signed in to change notification settings - Fork 6.6k
[cpp] add explicit files for deps #54311
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors how the Python build script sets the PYTHON3_BIN_PATH environment variable and makes the C++ package rule private by introducing an explicit filegroup for its outputs.
- Refactored
bazel_env
creation inpython/setup.py
to separate copying and assignment - Introduced
ray_cpp_pkg_files
filegroup and setray_cpp_pkg
genrule visibility to private incpp/BUILD.bazel
- Updated C++ test targets to depend on the new filegroup and removed a duplicate load statement
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
python/setup.py | Split environment copy and Python executable assignment |
cpp/BUILD.bazel | Added ray_cpp_pkg_files filegroup, made ray_cpp_pkg private, updated test data references |
python/setup.py
Outdated
@@ -528,7 +528,8 @@ def build(build_python, build_java, build_cpp): | |||
) | |||
raise OSError(msg) | |||
|
|||
bazel_env = dict(os.environ, PYTHON3_BIN_PATH=sys.executable) | |||
bazel_env = dict(os.environ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider using os.environ.copy()
instead of dict(os.environ)
for clearer, more idiomatic environment copying.
bazel_env = dict(os.environ) | |
bazel_env = os.environ.copy() |
Copilot uses AI. Check for mistakes.
@@ -109,12 +110,18 @@ cc_binary( | |||
}), | |||
) | |||
|
|||
genrule( | |||
name = "ray_cpp_pkg", | |||
filegroup( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] You may want to include the ray_cpp_pkg
genrule output (ray_cpp_pkg.out
) in the ray_cpp_pkg_files
filegroup so that any downstream dependencies needing the manifest have access to it.
Copilot uses AI. Check for mistakes.
1ea8026
to
0a48527
Compare
and make `ray_cpp_pkg` private visibility Signed-off-by: Lonnie Liu <[email protected]>
8223e5b
to
9d35c9a
Compare
also make `ray_cpp_pkg` private visibility Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: elliot-barn <[email protected]>
also make
ray_cpp_pkg
private visibility