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

Colcon fails to build valid PEP 517 projects #208

Open
rotu opened this issue Aug 14, 2019 · 15 comments
Open

Colcon fails to build valid PEP 517 projects #208

rotu opened this issue Aug 14, 2019 · 15 comments
Labels
enhancement New feature or request

Comments

@rotu
Copy link
Contributor

rotu commented Aug 14, 2019

As mentioned on colcon/colcon-ros#66 , colcon fails to build valid python packages that use pyproject.toml. This includes packages that want to use a setup.cfg without a corresponding setup.py.

$  colcon build --packages-select test_project
[0.403s] ERROR:colcon.colcon_core.package_identification:ROS package '/home/dan/ros2_ws/src/test_project' with build type 'ament_python' has no 'setup.py' file
[0.405s] WARNING:colcon.colcon_core.package_selection:ignoring unknown package 'test_project' in --packages-select
                     
Summary: 0 packages finished [0.27s]
@dirk-thomas dirk-thomas added the enhancement New feature or request label Aug 14, 2019
@rotu
Copy link
Contributor Author

rotu commented Oct 2, 2019

I tried implementing this, but realized that PEP-517 doesn't specify and pip doesn't implement installing pyproject.toml projects in editable mode. I'm opting not to move forward until editable mode can be cleanly implemented to support colcon build --symlink-install

pypa/pip#6950

@dimaqq
Copy link

dimaqq commented Jan 16, 2022

I can try to work on this... pyproject.toml is kinda generic, I think it still depends what tool is used.
poetry certainly allows installing the package in the editable mode.
Perhaps it makes sense to narrow the scope to specific tool(s).

@dimaqq
Copy link

dimaqq commented Jan 18, 2022

Re: editable mode, there's PEP-660 and poetry already supports that (in master branch... not sure if it has landed):
https://www.python.org/dev/peps/pep-0660/
python-poetry/poetry#34

@dimaqq
Copy link

dimaqq commented Jan 18, 2022

So... for today, I could only hope to get not symlink installs to work.
The editable installs have to wait for python-poetry/poetry#5056

Meanwhile, I think that non-editable installs can be made generic using the pypa/build package.

@dimaqq
Copy link

dimaqq commented Jan 18, 2022

I tried implementing this, but realized that PEP-517 doesn't specify and pip doesn't implement installing pyproject.toml projects in editable mode. I'm opting not to move forward until editable mode can be cleanly implemented to support colcon build --symlink-install

pypa/pip#6950

New enough pip now supports PEP-517 in editable mode, but that requires the built tool to export build_editable hook.
poetry doesn't today, I should find some other build system that does... maybe flit?

@dimaqq
Copy link

dimaqq commented Jan 19, 2022

I was able to craft 2 packages that pip==21.3.0 (or later, latest is 21.3.1) can install in the editable mode:

I think this is good enough wrt. editable installs via pyproject.toml PEP-517/PEP-660
I'll proceed to hack the core :)

@baszalmstra
Copy link

@dimaqq How is it going? Can I assist in any way?

@dimaqq
Copy link

dimaqq commented Mar 2, 2022

@baszalmstra sorry I don't have time to actively work on this right now.

Good news, there's a poetry-core release that supports editable mode ;-)
(no need for the hack where build-system was set to the master branch github reference)

Someone needs to make a PR against this repo.

My plan was to:

  • add the pep517 dependency to setup.cfg
  • use it in colcon_core/task/python/build.py
  • update colcon_core/package_identification/python.py to detect pyproject.toml as alternative to setup.py
  • somehow propagate the data through the project... TBH, I got mired in all the PythonPackage*** classes

@dimaqq
Copy link

dimaqq commented Mar 25, 2022

Kunal says:

Anyways, for pyproject_toml support, you might be looking in the wrong place. What you need to do is add an extension point for package_identification and package_augmentation in colon. See how this repo works: https://github.com/colcon/colcon-python-setup-py
You'll end up with a colcon-python-pyporoject-toml repo or similar with 2 extensions. On installing, these will hook into colcon extension points via options.entrypoint (https://github.com/colcon/colcon-python-setup-py/blob/master/setup.cfg#L60)

@mikepurvis
Copy link
Contributor

I'm very interested in this too— I agree that it should in principle be possible to implement this as a separate plugin that detects pyproject.toml and invokes some underlying tool (pip, pypa/build, whatever) as an alternative to what is currently done here with invoking setup.py directly.

However, given that those tools can also handle conventional setup.py-type packages, it's not a slam-dunk that colcon should have separate implementations for the two cases. Maybe the best way forward would indeed be to make colcon-core itself detect and work with all PEP 517 projects?


Outside of implementation choices, there's also a deeper question about how much work colcon should be doing around dependency awareness— like, if I have two poetry projects with a dependency between them (A -> B) in the same colcon workspace, is that something we'd expect colcon to actually handle, eg arranging for A at runtime to "see" B? If so, what would be the mechanism for it to work? Certainly thinking about how Poetry manages a centralized pool of venvs in ~/.cache/pypoetry, it is far from clear how colcon could inject itself into that, even less so in a way that would be generic across the various pyproject build tool options. All of this can be sidestepped somewhat by just using pypa/build which is purely build and doesn't do any of the dependency resolution, but then what's the point?

Thinking more about some of the reasons someone might want to use Poetry with ROS in the first place, its philosophy of separate per-project venvs might actually be a feature rather than a bug— imagining instead a workspace containing A -> [email protected] and B -> [email protected], it could be quite desirable that the Poetry-created executables for A and B internally set their sys.path values to "see" different instances of C, and in that world, there isn't an answer for what to do if C is suddenly part of the workspace as source. Maybe the answer is that if you're using Poetry, then you understand the implications of what it's doing, and it's up to you to update your in-source dependency specification to use path where desired?

@cottsay
Copy link
Member

cottsay commented Mar 27, 2023

I've just made my colcon-python-project extensions public: https://github.com/colcon/colcon-python-project/tree/devel
They were previously private because we wanted to do some more testing before migrating those extensions into colcon-core to eventually become the default but we didn't want folks to take a dependency on what is explicitly prototype code.

The long term plan is to switch colcon-core to use "standards-based" tooling exclusively, and move the existing functionality into a new extension to support down-level platforms that can't support the new extensions (RHEL 8, Bionic, etc).

It should be noted that these extensions implement colcon as a "build frontend", so we aren't wrapping pip or any of the other build frontends and aren't coupled to any of their behaviors and limitations. This should allow use of any build backend, though I've only tested poetry, setuptools, and flit.

One scenario that isn't 100% supported yet is using a build backend that is built as part of the same colcon workspace itself. I've been toying with introducing a build backend for Ament, and while colcon is able to bootstrap that package itself, we can't flesh out the full dependency graph without first having built the build backend package, so there is a bootstrapping problem. If you build the backend in a separate workspace, the problem goes away. I'm sure there are other limitations to my current approach, but that one is worth calling out.

re: dependency awareness - my implementation continues colcon's pattern of being aware of only the dependencies which are part of the colcon workspace.

To use the prototype package on a plain python project, it needs only be installed. To use it on ROS packages, you need to modify the colcon-ros package to use the new Python build extension instead of the setuptools-exclusive one. I'll make a branch with this change and post it here.

Since my internal efforts to get some testing done on these extensions have largely stalled, I'd appreciate any testing that you or anyone else can provide. Once we've settled on the general architecture, we can move the work to a PR against colcon-core.

I'm open to a synchronous discussion or demo on this work if it sounds helpful. In general, I'm much more optimistic than I was previously, but there are certainly still sharp edges here that we need to address, possibly as documentation and announcements.

@mikepurvis
Copy link
Contributor

@cottsay Ahh, awesome, thanks so much for sharing this work! I will definitely dig into it a bit when I have a moment to provide more thoughtful feedback. Looks like your approach builds wheels as an intermediate step, and therefore doesn't have a story for develop/editable mode? I believe Poetry at least does now allow direct editable installs, and I see you've got some seemingly-unused stuff related to editable in the hook_caller file, but I'm not fully following that as I probably don't have a grasp on how you're actually interfacing to the underlying build tool.

It does sound like there might still be room here for a Poetry-specific plugin that does let Poetry do some of its dependency- and lockfile-handling magic. My specific interest in Poetry for this (other than it being generally terrific) is because of how it can allow individual repos to spec their Python dependency versions and avoid having to manage that globally at the distribution/package-manager layer. And as a secondary concern, Poetry has a very nice story for integrating with Nix via poetry2nix, so for Clearpath's particular use-case, it would be possible to use "native" poetry operations for the user/workspace build, and then swap those out for the poetry2nix equivalents for the Nix sandboxed build (where network access is forbidden, and each dependency gets built to its own Nix store path).

I think it would likely come down to something like letting colcon handle in-workspace dependencies, and then having Poetry observe those on the colcon-managed PYTHONPATH and ignore them as being system-site-packages. Not at all clear to me how you enforce overall dependency coherence under this regime though— basically it gets back to the age-old issue of package.xml depend elements being deliberately ambiguous as to whether they're a distribution or system dependency, sigh.

Anyway, there are enough questions here that I probably need to crawl into my cave and do a little experimenting before submitting a more concrete proposal.

@cottsay
Copy link
Member

cottsay commented Mar 28, 2023

Looks like your approach builds wheels as an intermediate step, and therefore doesn't have a story for develop/editable mode? I believe Poetry at least does now allow direct editable installs, and I see you've got some seemingly-unused stuff related to editable in the hook_caller file, but I'm not fully following that as I probably don't have a grasp on how you're actually interfacing to the underlying build tool.

Right, this is mentioned in the TODO section of the README.md. I simply haven't implemented the hook yet. Per the PEPs, we should fall back to a non-editable install if the backend doesn't implement the editable hooks. I intend for this scenario to be supported.

It does sound like there might still be room here for a Poetry-specific plugin that does let Poetry do some of its dependency- and lockfile-handling magic.

I'm all for enabling new scenarios, but if we're going to pay the performance costs associated with adherence to the new APIs, we should reap the benefits and not couple ourselves to a single backend. As great as Poetry may be, I'd like to make sure colcon-core remains uncoupled. That said, I do think that implementing "helper" extensions for some specific backends is a great mechanism to improve performance. One of my main concerns with the new APIs is that we don't have an reliable and efficient way to retrieve metadata like the package name and version, and in a worst-case scenario, we actually need to build the package in order to get that information depending on how many of the optional hooks a backend implements. As with most things colcon, this would be extensible and best implemented outside of colcon-core.

I'd like to encourage anyone playing in this space to read the PEPs and understand where the responsibilities of the backend and frontend meet. A lot of the features you're discussing here are frontend features and many are even listed as explicitly out-of-scope for colcon's core packages: https://colcon.readthedocs.io/en/released/developer/design.html#explicitly-out-of-scope

@cottsay
Copy link
Member

cottsay commented May 6, 2023

We'll be discussing this topic at the Infrastructure Community Meeting on May 17: https://discourse.ros.org/t/infrastructure-community-meeting-2023-05-17/31268

@cottsay
Copy link
Member

cottsay commented Jun 16, 2023

I've issued a Call For Testing on a prototype package based on the PEPs discussed here: https://discourse.ros.org/t/call-for-testing-standards-based-python-packaging-with-colcon/32008

If you're interested in seeing support for modern package layouts, please take a moment to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

6 participants