Don't ignore setup.py even if a setup.cfg file exists

[rotu]

Even if using a setup.cfg, you need a setup.py to pull in its data, and that setup.py may have additional data in it which colcon was previously ignoring.

fixes #65

[rotu]

Rebased on latest

[rotu]

@dirk-thomas Any input on this? It seems like the code I removed is part of some unfinished feature (for _ in (1, ):) and it breaks things when you have a setup.cfg but your project name is specified in setup.py.

[rotu]

@dirk-thomas, @cottsay, @nuclearsandwich
Can I please get eyes on this?

[dirk-thomas]

Newer versions of setuptools do allow you to have a package with only a setup.cfg file and no setup.py file. So I don't think this patch works as is in all the cases.

[rotu]

Cool! I didn't realize that, but it seems such PEP 517 projects are broken under colcon anyway:

$  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]

A package without setup.py would also require a pyproject.toml file, which colcon completely ignores today anyway.

Do you still feel a change is necessary to this PR?

[rotu]

@dirk-thomas Do you feel a change is necessary to this PR to support packages without a setup.py file, or is it okay as is, since such packages are not supported by the build system? Please give me some direction here --- I'm willing to put in the leg-work. I can understand needing time to vet big or risky PRs, but this is neither and it's been stagnant for 2 months.

[rotu]

@dirk-thomas, @cottsay, @nuclearsandwich
I still feel like this is the correct change. Please advise how you want to move forward.

[dirk-thomas]

The colcon_core.package_identification.python.PythonPackageIdentification extension is responsible to identify Python packages with a setup.cfg file. The assumption of this extension is that all information related to the package are stored in this file and the setup() call in the setup.py file has no arguments.

The colcon_python_setup_py.package_identification.python_setup_py.PythonPackageIdentification extension is responsible to identify Python packages with a setup.py file. The assumption of this extension is that all information related to the package are passed as arguments to the setup() call. As such there should be no setup.cfg file (if there would be this extension wouldn't be able to provide the information from that file).

Atm the identification logic for ROS Python packages chooses between these two approaches based on what files are present - if there is no setup.cfg file then use the second approach, otherwise use the first.

So changing the logic as in this PR - to always use the second approach - would a) break packages using the first approach as well as b) wouldn't make the information from the setup.cfg file available as metadata (depending on what information have been moved from setup arguments to the config file it might not have any effect or break the package). Therefore I don't think the current patch is a viable change.

The question is how can we distinguish these different package types better?

[rotu]

The assumption of the project is wrong - project information is not "either in a setup.cfg or a setup.py file". A package can have arbitrary code in its setup.py file which may use setuptools or distutils or any other packaging tool, may pull in data from setup.cfg, distutils.cfg, and/or pydistutils.cfg, and may override data from those files in arbitrary ways.

For instance, demo_nodes_py has a setup.cfg file but also has a nontrivial amount of metadata in its setup.py.

There is an easy rule to distinguish the two types of packages - the presence of a pyproject.toml file. Currently, all packages should be of the first type (and pulling in the setup.py should suffice to pull in all project metadata) - you can check this by running the following:

from pathlib import Path
for p in Path('/opt/ros').rglob("setup.cfg"):
	if not((p.parent/'setup.py').is_file()):
		print(p)

With non-PEP-517 projects, a setup.cfg file is always accompanied by a setup.py file (and the build can also be affected by . With PEP-517 projects, you must parse the pyproject.toml and it may or may not do anything with the setup.cfg project based on the toml. (for instance, the project name would probably go in the toml, not the cfg).

[dirk-thomas]

The assumption of the project is wrong - project information is not "either in a setup.cfg or a setup.py file". A package can have arbitrary code in its setup.py file which may use setuptools or distutils or any other packaging tool, may pull in data from setup.cfg, distutils.cfg, and/or pydistutils.cfg, and may override data from those files in arbitrary ways.

I wasn't describing how it should be or what would be correct - I was only describing the currently implemented logic.

For instance, demo_nodes_py has a setup.cfg file but also has a nontrivial amount of metadata in its setup.py.

And atm only the arguments to the setup() call are considered in colcon-ros - the information from the setup.cfg is being ignored for the step of the package identification.

There is an easy rule to distinguish the two types of packages - the presence of a pyproject.toml file. Currently, all packages should be of the first type (and pulling in the setup.py should suffice to pull in all project metadata) ...

That is simply incorrect. Please check any colcon package itself (e.g. in this repo). All the information is stored in the setup.cfg file: package name, dependencies, entry points, metadata. From introspecting the arguments of the setup() call in the setup.py file non of these can be determined. And that is exactly why a package like this can't be processed by the second extension (provided by colcon_python_setup_py).

With non-PEP-517 projects, a setup.cfg file is always accompanied by a setup.py file (and the build can also be affected by . With PEP-517 projects, you must parse the pyproject.toml and it may or may not do anything with the setup.cfg project based on the toml. (for instance, the project name would probably go in the toml, not the cfg).

That is technically correct - PEP 517 support can maybe be added in the future - but that is not the point here. A setup.py file simply isn't introspectable when all the information is stored in a setup.cfg file. And that is why two separate extensions exist atm.

[rotu]

Ooooooooooooooohhhhhhhhhhhh. I figured this totally wrapped the call to setup.py and got the resulting egg info. I didn't realize that we don't capture the actual egg info emitted by setup.py - just the arguments to the setuptools.setup call. I think that gives me enough direction to rework this PR.

[rotu]

Closing in preference to colcon/colcon-core#215