Add ResolvedRequirement and remove sdist_root_dir from internal APIs?

[tiran]

A lot of APIs pass ctx, req, version, and source_root_dir around. We could make our implementation easier to understand by introducing a ResolvedRequirement type and removing source_root_dir.

Instead of separate req: Requirement and version: Version arguments, we should combine both into a ResolvedRequirement object. This makes it obvious that a function expects a requirement that has been resolved to a particular version. We can also extend the type, e.g. include a canonicalized_name and override_name attribute.

As far as I can tell, the source_root_dir parameter is redundant in almost all cases. unpack_source() ensures that it is always equal to ctx.build / f"{req_name}-{version}" / f"{req_name}-{version}", where req_name is the normalized override name from req.name. A user plugin could return a different path, though. I would rather make the naming convention a hard requirement for override plugins.

I have a prototype of a ResolvedRequirement implementation that wraps a requirement and version object:

>>> import pathlib
>>> from fromager.req import ResolvedRequirement
>>> from packaging.version import Version
>>> from packaging.requirements import Requirement

>>> rr = ResolvedRequirement(Requirement("egg-spam>7"), Version("42.0.0"))
>>> rr
<ResolvedRequirement('egg-spam>7', 42.0.0)>
>>> rr.req
<Requirement('egg-spam>7')>
>>> rr.version
<Version('42.0.0')>
>>> rr.name
'egg-spam'
>>> rr.specifiers
<SpecifierSet('>7')>
>>> rr.canonicalize_name
'egg-spam'
>>> rr.override_name
'egg_spam'
>>> str(rr)
'egg-spam==42.0.0'
>>> pathlib.Path("build-dir") / rr / rr
PosixPath('build-dir/egg_spam-42.0.0/egg_spam-42.0.0')

[shubhbapna]

+1 we might still need to keep sdist_root_dir though because a user can choose to override prepare_source and return a sdist_root_dir that does not comply with our form

[tiran]

We could require that the user returns the expected sdist_root_dir. Is there any good reason we have to support deviations?

[dhellmann]

We're already standardizing the sdist directory name, and that standardization broke sdist rebuilding (#430). What if we just went all the way, and always put the source code in a directory called src under the unpack directory? Then that's one less source of variation.

[shubhbapna]

What if we just went all the way, and always put the source code in a directory called src under the unpack directory? Then that's one less source of variation.

So sdist_root_dir will point to unpack_directory/src ? But the user can still control the unpack_directory yes?

[shubhbapna]

We could require that the user returns the expected sdist_root_dir. Is there any good reason we have to support deviations?

I don't any reason to support deviation. We are already doing some sort of type checking on the return values of the plugins, we could add this restriction as well

[dhellmann]

What if we just went all the way, and always put the source code in a directory called src under the unpack directory? Then that's one less source of variation.

So sdist_root_dir will point to unpack_directory/src ? But the user can still control the unpack_directory yes?

We have work-dir and then under that a directory named based on the package and version, so work-dir/foo-1.2.3. Today, inside that, we have another redundantly named directory also based on the package and version (work-dir/foo-1.2.3/foo-1.2.3). I'm suggesting that instead of using name-version for that dir, we just use "src", so we would have work-direct/foo-1.2.3/src.

[shubhbapna]

Ah that makes sense. Yeah I think that will make our lives easier