-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature/examples repo #43
Conversation
@finn-ball @hofbi PTAL. |
.bazelrc
Outdated
@@ -26,6 +26,9 @@ build --heap_dump_on_oom | |||
# Speed up all builds by not checking if output files have been modified. | |||
build --noexperimental_check_output_files | |||
|
|||
# This is a mandatory flag. | |||
build --@rules_python//python/config_settings:python_version=3.10 |
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.
If you add the default = True
attribute to the python toolchain in the MODULE
file, then you don't need this, right?
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.
Didn't change the toolchain in rules_ros. The flag isn't needed in rules_ros but it's needed in the examples repo.
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.
Why is this required in the examples repo?
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.
Otherwise Bazel can't figure out that pip deps are indeed for Python 3.10 toolchain. E.g. I get:
ERROR: /home/user/.cache/bazel/_bazel_user/6e0e647a7496b7382203f7000f7beb27/external/rules_python~~pip~rules_ros_pip_deps/empy/BUILD.bazel:27:6: configurable attribute "actual" in @@rules_python~~pip~rules_ros_pip_deps//empy:pkg doesn't match this configuration: No matching wheel for current configuration's Python version.
The current build configuration's Python version doesn't match any of the Python
wheels available for this wheel. This wheel supports the following Python
configuration settings:
//_config:is_python_3.10
To determine the current configuration's Python version, run:
`bazel config <config id>` (shown further below)
and look for
rules_python//python/config_settings:python_version
If the value is missing, then the "default" Python version is being used,
which has a "null" version value and will not match version constraints.
This instance of @@rules_python~~pip~rules_ros_pip_deps//empy:pkg has configuration identifier aaa66e7. To inspect its configuration, run: bazel config aaa66e7.
For more help, see https://bazel.build/docs/configurable-attributes#faq-select-choose-condition.
ERROR: Analysis of target '//chatter:talker_tests' failed; build aborted: Analysis failed
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.
If this is required for the examples repo only, would it make sense to have a .bazelrc
for the examples repo only to avoid this for the main repo?
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.
I don't want to maintain two .bazelrc files, that's why I synmlink-ed the rules_ros2 repo one to the examples repo. The flag doesn't harm the main repo.
@@ -0,0 +1,29 @@ | |||
alias( |
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.
Are these aliases to ensure they are built as I don't see what in the examples depends on them. If so, could be worth adding a comment as to why they're here.
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.
It's more for convenience, referenced some of them in the readme for the chatter examples.
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.
Added docs.
], | ||
) | ||
|
||
symlink( |
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.
I don't like to approach in particular, but until py_binary starts to have a single file in DefaultInfo, I am afraid we'll need this.
In particular: https://github.com/bazelbuild/rules_python/blob/main/python/private/common/py_executable.bzl#L767: DefaultInfo.files should be just the executable, the rest should go to runfiles. For backwards compatibility reasons, this functionality needs to go under a new incompatible flag.
Co-authored-by: Finn <finn.ball@codificasolutions.com>
…nto feature/examples_repo
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.
Regarding your third bullet point in the PR description about overriding/extending the requirements, @finn-ball already created this PR for rules_python. We are still discussing why this is required, so maybe we can use this as an example. Feel free to join the discussion there.
.bazelrc
Outdated
@@ -26,6 +26,9 @@ build --heap_dump_on_oom | |||
# Speed up all builds by not checking if output files have been modified. | |||
build --noexperimental_check_output_files | |||
|
|||
# This is a mandatory flag. | |||
build --@rules_python//python/config_settings:python_version=3.10 |
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.
Why is this required in the examples repo?
@@ -0,0 +1,51 @@ | |||
module(name = "rules_ros_examples") | |||
|
|||
bazel_dep(name = "rules_python", version = "0.34.0") |
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.
If you now specify your own python toolchain, wouldn't it be possible to use a different python version?
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.
It won't let me reply to the above .bazelrc
question but basically because no toolchain is specified as default in the example MODULE.bazel file, you have to specify it as a config flag in the .bazelrc
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.
Would it help if we specify a toolchain here?
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.
If I add the following to examples/MODULE.bazel:
python = use_extension("@rules_python//python/extensions:python.bzl", "python")
python.toolchain(
is_default = True,
python_version = "3.10",
)
and remove the config flag from .bazelrc: that works for the examples repo but I get:
DEBUG: /home/user/.cache/bazel/_bazel_user/6e0e647a7496b7382203f7000f7beb27/external/rules_python~/python/private/python.bzl:46:10: WARNING: Ignoring toolchain 'python_3_10' from module 'rules_ros': Toolchain 'python_3_10' from module 'rules_ros_examples' already registered Python version 3.10 and has precedence
IMO, the flag in .bazelrc is a cleaner solution.
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.
But isn't this what we want? Being able to override the Python toolchain used?
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.
Didn't have time to investigate how this would work. If you have some time, give it a shot. But, yes, it would be nice if toolchain override could work.
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.
Ok fine with me. Let's try that and once it is merged I will try to update in our codebase and report back if there are any errors.
The PR converts examples to a separate repo -- the idea is to illustrate how one would include rules_ros into their own projects.
Some notes: