-
Notifications
You must be signed in to change notification settings - Fork 388
feat(build): consume package.xml manifest for ROS
#4820
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
feat(build): consume package.xml manifest for ROS
#4820
Conversation
ruben-arts
left a comment
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.
Generally looks good, I've not tested it yet, but I have a few changes I think would make this even better.
|
I would approve with my changes added |
|
@nichmor we should add a test for this at https://github.com/prefix-dev/pixi-build-testsuite |
|
@nichmor @remimimimimi @ruben-arts I don't really get why you introduce |
|
@Hofer-Julian because that then points to the workspace. As it always needs a workspace manifest |
Sure, but the way that workspace manifests are detected is well-defined. There's no need for something like this: pixi build --manifest-path /path/to/workspace-manifest \
--package-manifest /path/to/package-manifestAs a user, this would make the most sense to me, since I am already used to this flag: pixi build --manifest-path /path/to/package-manifest |
If we allow |
|
@nichmor and I discussed this. Tbh, I still don't really understand why we'd support specifying a different workspace manifest than the one that belongs to a package manifest, but I appreciate that I might be missing use cases. What is really important to me though that we provide good defaults. So if I am at a directory that contains a workspace manifest and I do the following It should find the workspace manifest of You can then override this by specifying We should also add a couple of tests to test the behaviour |
package.xml manifest for ROS
|
Sorry for all the confusion that I created with my initial proposal, @Hofer-Julian explained his point well and we discussed the following changes. Let's remove We should change So it would look like this with the current supported design: pixi build --path src/pixi.toml
pixi build --path src/pyproject.toml
pixi build --path src/recipe.yaml
# or for any of the above
pixi build --path src/
# For package.xml we only support direct definition
# Similar to the TOML definition:
# package = { path = "src/package.xml" }
pixi build --path src/package.xmlWe don't want to extend this list of directly supported package manifests, but instead add a pixi build --path cargo.toml --backend pixi-build-rust
pixi build --path pyproject.toml --backend pixi-build-python
pixi build --path package.xml --backend pixi-build-ros |
|
temporary blocked by prefix-dev/pixi-build-backends#451 |
|
made the changes - and added tests for multiple scenarios ( implicit /explicit recipe.yaml, and implicit/explicit package.xml) |
ruben-arts
left a comment
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 doesn't work when I do:
autoware_launch/autoware_launch on main [!]
❯ ls
CHANGELOG.rst CMakeLists.txt README.md autoware_launch.drawio.svg config launch package.xml rviz test
autoware_launch/autoware_launch on main [!]
➜ pixi build --path package.xml
Error: × unable to canonicalize ''
╰─▶ No such file or directory (os error 2)
it does work with ./package.xml
tested and fixed the problem with referencing the same package.xml Also added new test cases here: prefix-dev/pixi-build-testsuite#94 |
| with: | ||
| repository: prefix-dev/pixi-build-testsuite | ||
| repository: nichmor/pixi-build-testsuite | ||
| ref: feat/change-manifest-path-to-path-in-pixi-build |
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.
temporary until we merge the new test-cases
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.
Our current workflow is that we only verify that things work in unison by adding a .env.ci file at the testsuite repo. You can revert this change
ruben-arts
left a comment
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 works for me now! Thanks

Overview
Allow providing a
--pathoption that you can point to a package to be built.Currently supported file formats are: recipe.yaml (rattler-build backend), package.xml ( ros backend ) and
pixi.tomlitself.In this case, from your current/or upper
workspacechannels will be taken, and ourDiscoveryBackendwill try to find your package from--path.note:
package.xmlshould be explicitly set in--path.How to test it?
You can do
pixi run install-debug-as pixi-xmland then in the root of https://github.com/ruben-arts/ros_workspace
run
pixi-xml build --package-manifest src/navigator/package.xml