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

Build farm doesn't handle pip test dependencies properly #895

Open
ggaessler opened this issue Jul 20, 2021 · 12 comments
Open

Build farm doesn't handle pip test dependencies properly #895

ggaessler opened this issue Jul 20, 2021 · 12 comments

Comments

@ggaessler
Copy link

I have python3-virtualserialports-pip as a test dependency in a package, however the build farm tries to resolve the pip package as an apt package, see:
https://build.ros.org/job/Npr__nmea_navsat_driver__ubuntu_focal_amd64/8/console
python3-virtualserialports-pip
ros-drivers/nmea_navsat_driver#130

Is this to be expected? How could that be fixed?

The pip package was added to the rosdistro index with the -pip suffix as requested, so it should be obvious that it should be installed via pip, right?
See: ros/rosdistro#29587

@tfoote
Copy link
Member

tfoote commented Jul 27, 2021

Pip and other alternative installers are only supported when building from source.

To be submitted to be built on the buildfarm all dependencies need to be available via native packages such as debians and rpms. It would be possible to work around the PR jobs and install the pip dependencies. However that would then lead to people believing that if they released their package it would work. The primary role of the PR job is to validate packages are ready for release and as such they need to have native packages available for the build pipeline. And the official build pipeline uses the standard debian pipeline for building which needs dependencies to resolve.

If someone would like to extend the PR job to do this we would want an explicit flag to say use the permissive mode which adds pip and other alternative installer support and also can let our other tools know not to try to build binary packages. So that maintainers and users won't be suprised if they try to release it.

@musamarcusso
Copy link

but also for test dependencies? At least when I run bloom-generate rosdebian the test dependencies aren't included as the Debian package's dependencies.
The case that pip packages shouldn't be build or exec dependencies I can understand.

@ggaessler
Copy link
Author

@musamarcusso thanks for emphasizing that test dependencies should be handled differently.
@tfoote thanks for your answer, but what is your opinion on test dependencies which won't end up in Debian packages anyways?

@tfoote
Copy link
Member

tfoote commented Aug 17, 2021

We've been discussion how the test dependencies should be handled. Part of the debian pipeline is to run the tests during the packaging process. Usually if the tests fail to pass on the build used for packaging the packaging process will abort. We have disabled that check on the builds. But in general I don't think that we should accept pip dependencies for any dependency even test dependencies of any packaged tool.

Pip dependencies are much less robust and curated, change a lot more and have been through a lot less QA processes. If we're packaging something I am comfortable saying that all your dependencies need to be available as apt dependencies. That way users can always test and validate their packages using debian packages w/o needing to create a pip overlay which has many times tripped up users due to not automatically updating in sync with the apt packages. This also avoids many layers of complexity on the buildfarm where we would start needing to support mixing each native packaging mechanism and pip overlays which depending on how they're implemented may give different results than the user will experience.

@ggaessler
Copy link
Author

@tfoote thanks for your answer. I think we're on the same page that pip dependencies shouldn't be build or runtime dependencies.
But again, we're talking about test dependencies, which never get packaged and shipped with the Debian packages. That negates basically all cons of pip which you mentioned :). If pip wouldn't be allowed as test dependency then many concepts wouldn't make any sense to me at all, e.g.:

  • Why could we then add pip dependencies to rosdistro if we weren't allowed to use them at all? (see. e.g. Add virtualserialports to python.yaml ros/rosdistro#29587)
  • Why can rosdep then install pip dependencies?
  • Why does catkin lint in strict mode then not throw an error when including pip packages?

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Aug 17, 2021

I'm not @tfoote, but this has been discussed over at ROS Answers a few times, so:

  • Why could we then add pip dependencies to rosdistro if we weren't allowed to use them at all? (see. e.g.

to allow people to easily install dependencies for packages to-be-built-from-sources in a (local) workspace.

  • Why can rosdep then install pip dependencies?

See previous.

  • Why does catkin lint in strict mode then not throw an error when including pip packages?

Because the author of that (stand-alone) tool apparently hasn't made it do that -- which makes sense, as the limitation is the buildfarm, it's not a general limitation or ban. If catkin_lint would consider that an error, it would become really annoying for packages which are not intended to be released through ros_buildfarm, but are built from source in a (local) workspace.

@musamarcusso
Copy link

I think he meant only in case we run catkin_lint --strict -W2. There are many things only thrown as a warning only when calling catkin_lint and this would be a use-case for that. But it's a separate package apparently, maybe we can open an issue there to ask.
Would this mean we just give up on the contribution? I don't really see another way to run these tests.
It would be a shame, but I think there is nothing we can do apart from building this package only on the internal buildfarm here for internal use as a Debian package and maintain the fork.

@gavanderhoorn
Copy link
Contributor

I'm not really in a position to say anything about the buildfarm proper.

Personally I would not be against it supporting test_depends for CI runs and source builds. Since ros-infrastructure/rosdep#789 it would actually be possible.

It will require some effort though, and I assume that's what's going to be the bottleneck for this 'ask'.

@ggaessler
Copy link
Author

@gavanderhoorn CI runs using ros-tooling (without package generation) work perfectly fine with with pip test dependencies. See https://github.com/boschresearch/nmea_navsat_driver/runs/3056126445
It's just the ROS buildfarm no accepting it.
I just wanted to push the features from our forks back to the upstream repos. But it seems like I would have to maintain our forks and use the Debian builds from our internal build farm instead.

@gavanderhoorn
Copy link
Contributor

I'm not sure what you're trying to say. The buildfarm doesn't use ros-tooling.

@ggaessler
Copy link
Author

@gavanderhoorn I didn't indent to say that the buildfarm would use ros-tooling. Just that there exist ROS tools to do the job, but the buildfarm uses a different pipeline. Thanks for the clarifying inputs.

@tfoote
Copy link
Member

tfoote commented Aug 20, 2021

If you'd like to contribute this capability we'd be happy to accept it. However please do make sure it's an option that we can turn off or on so that we have the choice whether to enable it on our buildfarm with the above stated risks etc. As well as such that we won't expend build time trying to build the debian packages.

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

No branches or pull requests

4 participants