Open lalten opened 7 months ago
Current state is that things are complaining about boost. I think rather than trying to use the WORKSPACE-only rules_boost it would be best to switch over to https://github.com/bazelbuild/bazel-central-registry/pull/1280#issuecomment-1902491968 (once that's ready).
Boost is used at a single place https://github.com/mvukov/rules_ros2/blob/68a2b811d40bbe1713325a2fdd1c99993ae1f7c9/repositories/resource_retriever.BUILD.bazel#L13 (IMO, that package is outdated but still needed by the foxglove bridge). The receiver class is fairly small and contained last thing I remember. So, I'd suggest to patch it and use std::shared_ptr; then we can remove the boost dep. I can try to do this over the weekend or you can try to do it if you have some spare time. I'd target this change against the main branch to keep this PR as small as possible.
Here's the issues/todos as of now:
Getting closer...
Still getting this from the WORKSPACE version of asio
but not in the MODULE.bazel one.
external/asio/include/asio/ssl/detail/openssl_types.hpp:23:10: fatal error: 'openssl/conf.h' file not found
#include <openssl/conf.h>
^~~~~~~~~~~~~~~~
1 error generated.
The reason is that Bzlmod asio is configured as header-only: https://github.com/bazelbuild/bazel-central-registry/blob/main/modules/asio/1.28.2/patches/add_build_file.patch
But in WORKSPACE we use the checked-in asio.BUILD that has https://github.com/mvukov/rules_ros2/blob/4b8441a96ccd5fba5cf613f9637de28f26928a13/repositories/asio.BUILD.bazel#L13-L15 to switch from header-only to lib compilation and (because of that) has to add https://github.com/mvukov/rules_ros2/blob/4b8441a96ccd5fba5cf613f9637de28f26928a13/repositories/asio.BUILD.bazel#L18
CI is green with this change, but that may just mean that there are no tests that really need ssl :)
Not sure what's the better approach here. But IMO WORKSPACE and MODULE.bazel should both do the same thing.
We still need OpenSSL for libcurl, which is not on BCR yet.
State of things:
asio
's BUILD is switched to header-only mode to be the same as the Bzlmod versionThe numpy header thing is really annoying still and I haven't found a good way to work around this. Maybe we can just leave this for now.
I think things should be working now. Would be happy about a review @mvukov.
1e16de8 looks promising, thanks for the efforts. We shall wait for the new rules_python release, right?
1e16de8 looks promising, thanks for the efforts. We shall wait for the new rules_python release, right?
yup, it landed in https://github.com/bazelbuild/rules_python/commit/730a2e39bd2702910f28629d4583b3ec49f4ee5e, now we just need a release.
I'm still not happy with the iterating over Python versions in MODULE.bazel. That's basically https://github.com/bazelbuild/rules_python/issues/1708.
I'm still not happy with the iterating over Python versions in MODULE.bazel. That's basically https://github.com/bazelbuild/rules_python/issues/1708.
The requirements lock file in this repo is resolved with Python 3.10. So, using that with other Python probably won't work as there are packages with pre-compiled extensions. Thoughts:
WDYT?
rename the lock file to have suffix _3_10 + mention this in the docs such that people are aware of this.
There's also a multi_pip_parse
, although I've never used that.
It might be worth looking into alternative python dependency machinery, like rules_uv or rules_poetry or rules_pycross. I don't have much experience with those, but maybe they have some solutions for this.
define python Bazel dep as dev dep, forcing users of rules_ros2 to define their own dependency on rules_python -- like we do it in no-blzmod case.
I don't really like the user experience of that
It might be worth looking into alternative python dependency machinery, like rules_uv or rules_poetry or rules_pycross.
It seems like https://github.com/oxidase/rules_poetry is indeed a nice solution! I have a POC locally that seems to work.
One small challenge was that it doesn't expose a whl (because there isn't one anymore as the interpreter isn't resolved "yet") for numpy, so can't use the new whl_filegroup. I don't know how I haven't thought of this earlier, but you can totally just have a http_archive of https://github.com/numpy/numpy/tree/main/numpy/_core/include/numpy and inject a BUILD exposing the headers as cc_library.
I'll try and get the WORKSPACE version onto rules_poetry as well, and then that can already be landed separately. Once that's out the door I think there are no blockers left for the rules_ros2 bzlmod migration :)
Oh -- there is no Workspace version of rules_poetry! @oxidase, any chance you'd like to still add that?
@mvukov -- it would be a big change, but wdyt about just leaving Workspace behind and going full Bzlmod? Then this wouldn't be an issue.
I take back the thing about a numpy http_archive being a good idea. Looks like some needed includes are generated (https://github.com/numpy/numpy/blob/v1.26.4/numpy/core/meson.build#L608-L614) and not part part of the sdist release. So you have to know which interpreter to target.
@lalten can do it during the next month. Anyways appreciate any PRs to the repo
@mvukov -- it would be a big change, but wdyt about just leaving Workspace behind and going full Bzlmod? Then this wouldn't be an issue.
Just weighing in as a rules_ros2
user: The projects relying on rules_ros2
that I (help) maintain are all still on WORKSPACE
. Making them stay on an old version by dropping WORKSPACE
support is nothing I would welcome.
As for the issues with Python: I haven't really looked into bzlmod yet, so not sure I understand the full picture. But moving away from rules_python
would also be a big step and potentially lead to other issues that are hard to foresee. I made the switch from rules_python
to rules_pycross
in one project. While it looked great on first sight, there were a couple of surprising issues. I was able to work around those mostly, but I'm really not sure if for an upstream project as rules_ros2
this is a good idea.
Making it explicit that we rely on Python 3.10 by mentioning this in the docs and renaming the lock file sounds like an acceptable approach to me. Folks can still patch things if they know what they're doing. And unless they know what they're doing, they shouldn't mess with the Python version anyway, since ROS 2 Humble officially only supports 3.10.
Anyway, awesome effort going on here. Thanks a lot for that!
since ROS 2 Humble officially only supports 3.10.
Looks like I have to take back that part. Ubuntu 20.04 is still supported as a Tier 3 platform, so Humble will be kept Python 3.8 compatible. But I doubt that has much relevance, so the part about having to know what one is doing remains valid. ;-)
But moving away from rules_python would also be a big step and potentially lead to other issues that are hard to foresee.
Heads up about moving away from rules_python
. From my understanding internal Python rules are deprecated and are moved to rules_python
as Starlark-based definitions in Bazel 7. For example PyInfo is selected by config.enable_pystar with Starlark PyInfo
not compatible to builtin PyInfo
. Moving away from rules_python
will lead to issues like this with cryptic messages
ERROR: poetry-demo/BUILD.bazel:3:10: in deps attribute of py_binary rule //:main:
'@@rules_poetry~~poetry~poetry//:absl-py' does not have mandatory providers:
'PyInfo' or 'PyVirtualInfo'. Since this rule was created by the macro 'py_binary',
the error might have been caused by the macro implementation
Heads up about moving away from
rules_python
. From my understanding internal Python rules are deprecated and are moved torules_python
as Starlark-based definitions in Bazel 7.
My wording was not precise enough: Instead of just "Moving away from rules_python
", it should have been "Moving away from rules_python
for Python dependency management". The project I mentioned that now uses rules_pycross
also still uses rules_python
for things such as py_library
, but no pip_parse
or generated requirement
from rules_python
is used. That part is handled by rules_pycross
.
@lalten I've just pushed 0.3.6 release.
Added two examples:
@mvukov -- it would be a big change, but wdyt about just leaving Workspace behind and going full Bzlmod? Then this wouldn't be an issue.
Hm, no. Including me, there are folks around depending on nobzlmod.
Regarding
define python Bazel dep as dev dep, forcing users of rules_ros2 to define their own dependency on rules_python -- like we do it in no-blzmod case.
I don't really like the user experience of that
In main branch, noblzmod still, I left rules_python setup open to users; intentionally. I don't really care which Python folks are going to use. As long as users provide necessary stuff, this ruleset is going to work: rules_ros2_pip_deps in particular. On the pip requirements side, there is a set of necessary deps defined in requirements.txt and this repo provides a lock file for Python 3.10. Users happy with that, fine. Not so much? They can resolve the deps for an interpreter version they want to work with.
Now, what can happen in the current state of this PR, is that folks can have rules_ros2_pip_deps and their own my_repo_pip_deps. Let's say that both pip_deps repos have a package "foo" resolved at different versions (foo-1.3 and foo-2.4 for instance) and used as a dep (e.g. transitively) in the same Bazel target. Best case you have the same package shipped two times, worst case you get creepy runtime errors due to some version incompatibility (this would happen sooner or later). In an extreme case, rules_ros2_pip_deps is resolved for one Python version and my_repo_pip_deps for another. To avoid those issues, I strongly suggest this repo defines rules_python and rules_ros2_pip_deps as dev deps, forcing users defining rules_ros2_pip_deps in their own repo. This isn't documented yet (should be :) ), but folks should ideally have rules_ros2_pip_deps where they have both necessary packages for rules_ros2 and other packages they might need.
Those are just my thought, pretty much how I'd do it. I am very open to hear other suggestions and learn how users of rules_ros2 set up Python in their own monorepos.
I just learned about https://rules-python.readthedocs.io/en/latest/toolchains.html#pinning-to-a-python-version, that is likely what we want here.
(still need to update this PR with the rules_python release)
Things seem to work, the only drawback is that load("@rules_python//python:defs.bzl", "py_binary")
must become load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary")
, so there is a bunch of 3.10
hardcoded everywhere. While not pretty I think that would be acceptable
I am unsure what causes the test failures (or how to fix them), could you have a look @mvukov or @ahans ?
Fixed in https://github.com/mvukov/rules_ros2/pull/238/commits/435e95af9280a4cf584ac81a52118d0c9d5ababe
Things seem to work, the only drawback is that
load("@rules_python//python:defs.bzl", "py_binary")
must becomeload("@rules_ros2_pythons//3.10:defs.bzl", "py_binary")
, so there is a bunch of3.10
hardcoded everywhere. While not pretty I think that would be acceptable
Why do you think this is necessary? AFAIK rules_python
will use the default toolchain when using load("@rules_python//python:defs.bzl", "py_binary")
, so load("@rules_ros2_pythons//3.10:defs.bzl", "py_binary")
should be equivalent to registering the 3.10
toolchain as default.
Things seem to work, the only drawback is that
load("@rules_python//python:defs.bzl", "py_binary")
must becomeload("@rules_ros2_pythons//3.10:defs.bzl", "py_binary")
, so there is a bunch of3.10
hardcoded everywhere. While not pretty I think that would be acceptableWhy do you think this is necessary? AFAIK
rules_python
will use the default toolchain when usingload("@rules_python//python:defs.bzl", "py_binary")
, soload("@rules_ros2_pythons//3.10:defs.bzl", "py_binary")
should be equivalent to registering the3.10
toolchain as default.
right, but my understanding is that if the root repo uses a default != 3.10, the rules_ros2_pip_deps are not guaranteed to be compatible
Things seem to work, the only drawback is that
load("@rules_python//python:defs.bzl", "py_binary")
must becomeload("@rules_ros2_pythons//3.10:defs.bzl", "py_binary")
, so there is a bunch of3.10
hardcoded everywhere. While not pretty I think that would be acceptableWhy do you think this is necessary? AFAIK
rules_python
will use the default toolchain when usingload("@rules_python//python:defs.bzl", "py_binary")
, soload("@rules_ros2_pythons//3.10:defs.bzl", "py_binary")
should be equivalent to registering the3.10
toolchain as default.right, but my understanding is that if the root repo uses a default != 3.10, the rules_ros2_pip_deps are not guaranteed to be compatible
Are you sure that they will chosen in the first place? My understanding is that it will only use pip deps with the correct version. This is why most projects specify the same pip deps for many Python versions. IIUC when it won't find pip deps in the correct version, it will throw an error.
@lalten were you able to make progress on this PR?
@lalten were you able to make progress on this PR?
I'm working on this on and off when I find time, which I didn't since the last commit 🙈
No worries, thank you for your contribution. As far as I understand, you got everything working already, right? Maybe time to merge 👀 ?
Incomplete, work in progress 🚧