Add handling of multiple deps and different extras

[david-caro]

Very simple handling of the case where you have multiple requirement
definitions that are the same, but differ on the extras, like:

mypgk[extra1]
mypgk[extra2]

will be merged into:

mypgk[extra1,extra2]

Signed-off-by: David Caro david@dcaro.es

[piotr-dobrogost]

Isn't this the same feature as in pull #3198?

[david-caro]

looks the same, looking (it did not work for me locally though... so if it is, it might be broken)

[david-caro]

it actually seems to be avoiding the duplicates, that is, ignoring the duplicated entry, instead of merging it into the existing one.

[david-caro]

hmmm... might it be that the if condition is wrong there? it actually aggregates the new requirement extras and the existing one, but the if says:

             if (parent_req_name is None and existing_req and not
                    existing_req.constraint and
                    existing_req.extras == install_req.extras):

so it will never get in if the extras are different.

[david-caro]

sorry... I'm wrong, that if is to throw the exception

[david-caro]

hmm... so the merging of the extras only gets done if the existing_req.constraint is false, and the install_req.constraint is true:

https://github.com/pypa/pip/blob/master/pip/req/req_set.py#L272

                if not install_req.constraint and existing_req.constraint:
                    if (install_req.link and not (existing_req.link and
                       install_req.link.path == existing_req.link.path)):
                        self.reqs_to_cleanup.append(install_req)
                        raise InstallationError(
                            "Could not satisfy constraints for '%s': "
                            "installation from path or url cannot be "
                            "constrained to a version" % name)
                    # If we're now installing a constraint, mark the existing
                    # object for real installation.
                    existing_req.constraint = False
                    existing_req.extras = tuple(
                        sorted(set(existing_req.extras).union(
                               set(install_req.extras))))
                    logger.debug("Setting %s extras to: %s",
                                 existing_req, existing_req.extras)
                    # And now we need to scan this.
                    result = [existing_req]

Maybe that's the if that is not ok? Shouldn't it be that none of their .constraint is True? (I'll try to find out what the constraint actually means though).

[david-caro]

A quick'n'dirty test just changing the if condition seems to work ok, will investigate but any info is really appreciated 😉

[david-caro]

I think this is ready, added also a test for the special case.

[david-caro]

ping?

[david-caro]

ping?

[david-caro]

ping?

[xavfernandez]

This seems clean enough but this would (if I'm not mistakenà merge a pkg[extra1]<1 and a pkg[extra2]>=2 into a pkg[extra1,extra2]<1 (or >=2 depending on the resolution order).

This would not be much worth than pip's current behavior though.
Any opinion @pypa/pip-committers ?

[david-caro]

This pr already solves the issue of not doing the requirements merge at all, that already allows us to not have to run pip twice on deployment. Si I think that this already improves the current behavior.

Imo the handling of the versioning there might deserve another pull request.

[david-caro]

ping?

[pradyunsg]

remove this?

[pradyunsg]

You could just add simple as a dependency and use script.pip_install_local in the test, using the package names.

[david-caro]

I reused the already existing LocalExtras package. So, to change that, here would be:

# No need for path_to_url, HERE, DEP_PATH or DEP_URL

setup(
    ...
    install_requires=['simple', 'LocalExtras[Bar]'],
    ...
)

And then in the test something like:

result = script.pip_local_install('--no-index', '-f', data.find_links, to_install + '[shrubbery]')

?

(it's been quite a long time, so my memory is a bit rusty)

[pradyunsg]

Yes. Off the top of my head, the test would be:

result = script.pip_local_install(to_install + '[shrubbery]')

[pradyunsg]

It'd be nice if you could rebase this; there's a few questions I wanna ask but I'll wait for a response from you before asking them.

[pradyunsg]

Hi @david-caro!

Sorry for the lack of a response for all this while; pip's very short on developer time.

If you could rebase or merge master in this PR and add a news file, that'd be nice. :)

[david-caro]

I've rebased over latest master, it seems to fail on travis for different reasons though, any help is appreciated (will try to check closer, but might take me some time).

[pradyunsg]

@david-caro I think I broke the master CI build with merge of #4739. Sorry! :|

I've a fix for the faulty test in #4841.

[pradyunsg]

We've had a change in process; so instead of modifying NEWS.rst, you'd want to add a file in news/ directory. More about that here: https://pip.pypa.io/en/latest/development/#adding-a-news-entry

[pradyunsg]

Yes. Off the top of my head, the test would be:

result = script.pip_local_install(to_install + '[shrubbery]')

[pradyunsg]

@david-caro The tests are fixed on master. :)

[david-caro]

Updated and now passing the tests too.

[BrownTruck]

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

[pradyunsg]

Gonna go ahead and close this due to lack of activity here.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.