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

Configuration options to include packages from require-dev and suggests #70

Closed
Majkl578 opened this issue May 30, 2018 · 14 comments
Closed
Assignees

Comments

@Majkl578
Copy link

It seems that currently only packages used directly in require section are considered.
There are multiple cases when packages are optional dependency and as such are only present in suggest (and possibly also require-dev) section. These are currently reported as missing dependency, although they are not hard dependency.

@Ocramius
Copy link
Collaborator

This is exactly what this package is about: finding them and marking them as violation

@Ocramius Ocramius self-assigned this May 30, 2018
@Ocramius
Copy link
Collaborator

Duplicates #55

@Majkl578
Copy link
Author

Duplicate of "False positives on some bundle and classes" - you didn't really want me to find that issue as relevant report, did you? :)

finding them and marking them as violation

And it is exactly what is wrong, it's not a violation, it's expected. And there is no way to configure it (I don't consider symbol-whitelist as a sufficient solution, that's rather a hack).

@Ocramius
Copy link
Collaborator

you didn't really want me to find that issue as relevant report, did you? :)

No, I simply proactively link issues together.

And it is exactly what is wrong, it's not a violation, it's expected.

Not in this package - the design is specifically around preventing optional dependencies upfront.

A package such as doctrine/cache, for example, having multiple dependencies on multiple extensions, would never pass here, but would need to be split into doctrine/cache, doctrine/cache-memcache, doctrine/cache-redis and so on.

Again: this is by design. Optional dependencies are a mess and should be slowly removed over time. We had massive issues in dozens of packages in zendframework/zendframework because of this, and it's slowly being fixed by moving forward and removing dependencies completely or splitting them into separate additions. @weierophinney can maybe add something here, but if you have "optional dependencies" (which aren't really a thing, by the way), then this package is not for you.

@Majkl578
Copy link
Author

Majkl578 commented May 30, 2018

The description of this package clearly says:

A CLI tool to check whether a specific composer package uses imported symbols that aren't part of its direct composer dependencies

and

A CLI tool to analyze composer dependencies and verify that no unknown symbols are used in the sources of a package.

require-dev is also a (direct) dependency, suggest sort of too (suggesting a package implies that your code is aware of its (non-) existence).

Optional dependencies are a mess and should be slowly removed over time.

Not possible until Composer has a strategy to define package sets/unions and choosing defaults for them. Without that, you either end up with ton of useless dependencies at runtime or missing dependencies (i.e. no ORM mapping driver).

"optional dependencies" (which aren't really a thing)

Same as above, they are very much a thing, until package manager has a strategy to handle things better.

@Ocramius
Copy link
Collaborator

require-dev is also a (direct) dependency

No, because downstream consumers won't get it, so you are making holes in the dependency graph

suggest sort of too

Suggest was proposed to be removed from composer overall, since it is basically an advertisement section in composer.json, nothing more.

Same as above, they are very much a thing, until package manager has a strategy to handle things better.

Yes, but until then you won't have a tool that tells you anything about that. If you have a plan on how to force intersection/union of "optional" dependencies that is part of the composer SAT itself, this is a non-issue here, and the package will simply focus on preventing missing dependencies from leaking through to unaware (and affected) downstream consumers.

@Majkl578
Copy link
Author

because downstream consumers won't get it

Only applicable to libraries and production code.

Suggest was proposed to be removed from composer overall

You are talking about some uncertain distant future. I am on the other hand talking about real-world situation/problem that is found in basically any larger package.

this is a non-issue here

It's not an issue here, but it's a trouble caused by rejecting the state in which PHP ecosystem currently is and will be for a while. (Even if composer adds such thing tommorow, it would take years to consider as default).

then this package is not for you

I take this, okay. /me leaves

Sorry that I simply wanted to make it more flexible and configurable and thus usable by more consumers.

@Ocramius
Copy link
Collaborator

Ocramius commented May 30, 2018

Only applicable to libraries and production code.

This is specifically for that: why would you need such checks otherwise?
Your "require-dev" is about checking that your (to be) sources and your deployable binaries do work.
Your "require" section is about making sure your deployable binaries (or your consumers, if you are a library) have all dependencies.
This is a "require-dev" package aimed at isolating issues in incorrect "require" scope.

You are talking about some uncertain distant future. I am on the other hand talking about real-world situation/problem that is found in basically any larger package.

Work towards getting to an exit code 0. Exactly like with any legacy project with no tests or with architectural issues, this needs a lot of work and also a mental shift in how things are being handled regarding the dependency graph. We used to do the "optional dependencies" quite a lot until a few years back, and that in multiple packages: this simply increases the issues in the ecosystem, and leads to "hard to fix" scenarios where removing the optional dependency makes the package either harder to use or conflicting with other components. I suggested the correct approach to this in #70 (comment)

Sorry that I simply wanted to make it more flexible and configurable and thus usable by more consumers.

It's not about widening the user-base of the package, it is about ensuring that the invariants provided by "require" in your package and consumers of your package make sense to consumers and to your --no-dev installed package.

@weierophinney
Copy link

@Majkl578 From a maintainer's point of view, "suggest" is a nightmare.

Why?

Because somebody will go and install the package, try to use that optional feature, and then file an issue saying, "feature X does not work". Maintainers will calmly point out that the feature is optional, and requires a dependency marked in the "suggest" section. The user will say they did not see it, and perhaps that dependency should be required. The maintainer will argue, "the feature is optional, that's why the dependency is optional."

This gets more heated when the user points out, "well, the dependency is in require-dev!" Yes, yes, it is; just because it's optional does not mean it should not be tested.

And then there's problems when optional features require PHP extensions. Don't even get me started on that.

And round and round it goes.

In ZF, we are actively attempting to split out anything that is optional functionality, particularly if such functionality requires additional dependencies, and putting them into a separate package. We have a project in place for zend-cache to split out all adapters into their own components; we will be doing so with zend-db and its adapters, too; same goes for zend-paginator (which has a couple zend-db-specific adapters). Doing this allows each to specify any code dependencies, as well as PHP extension dependencies, so that installation will fail if any of those are unmet. (With Expressive, we already did this with regards to routers, template engines, and session backends, and we're doing it as well with upcoming authentication and authorization adapter packages.)

We can then "suggest" these additional packages, and document the packages we know about, so that you, as a consumer, can discover what else you may need to install to get the combination of functionalities that achieve what you need.

So, while you may argue packaging all optional dependencies is a "real-world situation/problem that is found in basically any larger package," what @Ocramius and I are arguing is the exact same thing: packaging anything "optional" in a larger package leads to confusion by end-users and difficulty in maintenance. It's better to split it out into more packages, each with a discrete set of requirements, and nothing optional.

@Ocramius
Copy link
Collaborator

Note: it doesn't need to be separate repositories, but the distributed package (for which "require" is the only important bit) shouldn't have optional deps in it

@Majkl578
Copy link
Author

This is specifically for that: why would you need such checks otherwise?

In a project, to make sure indirect dependency doesn't get bumped to next major, that would break my code depending on it (consider doctrine/annotations for example).

This is a "require-dev" package aimed at isolating issues in incorrect "require" scope.

And it's not doing so correctly, given the nature of "suggest". Note that I am not arguing whether suggests are good or bad, I am simply referring to current state of PHP ecosystem.

We used to do the "optional dependencies" quite a lot until a few years back, and that in multiple packages: this simply increases the issues in the ecosystem, and leads to "hard to fix" scenarios

Agreed, but it's irrelevant. We are not talking about your code, but any code. And any code usually does use "suggest".

It's not about widening the user-base of the package, it is about ensuring that the invariants provided by "require" in your package and consumers of your package make sense to consumers and to your --no-dev installed package.

Wouldn't get changed at all by adding an opt-in configuration option. OTOH would help projects (not libraries) in finding and properly exposing their dependencies.

From a maintainer's point of view, "suggest" is a nightmare.

I never said otherwise, did I? But forced "require"" is a nightmare for consumers. Look at Doctrine ORM, I don't even want to imagine the rant when YAML would be moved from "suggest" to "require". Yet you would want to check & validate such library to detect missing dependencies.
Also, when speaking about ORM, consider extracting mapping drivers out to separate repos. You always need at least one driver, but usually don't want all of them. Also you want your consumer to have usable ORM by default, not telling them "you have to also install XYZ or ABC".
This is impossible to solve currently with Composer.

@Ocramius
Copy link
Collaborator

Look at Doctrine ORM, I don't even want to imagine the rant when YAML would be moved from "suggest" to "require". Yet you would want to check & validate such library to detect missing dependencies.

Taking this example specifically:

composer require doctrine/orm-yaml-mapping

This is the correct resolution for such a scenario. If the doctrine/orm package uses Yaml somewhere, then it needs to explicitly "require": {"symfony/yaml":"^4"}. Whether consumers will have an additional dependency or not is less problematic than consumers having to install optional dependencies, or being burnt by an optional dependency that got inadvertently removed because another package dropped it (yes, this happens a lot too).

@Majkl578
Copy link
Author

In such case, which driver will I get when I simply do composer require doctrine/orm?
None?
One that maintainers chose as default (and which I might not ever use)?

@Ocramius
Copy link
Collaborator

Ocramius commented May 30, 2018

In such case, which driver will I get when I simply do composer require doctrine/orm?

The ones that can work with the given set of "require". The PHP driver would work out of the box, the annotation one would require doctrine/annotations (which is there), and for the XML driver you'd either add "require": {"ext-xml": "*"} or split it out like the yaml driver.

One that maintainers chose as default (and which I might not ever use)?

This is a trade-off that comes with most libraries, and obviously needs discipline from a library author's perspective.

Another possible alternative is to not ship any drivers at all, and instead use "require": {"doctrine/orm-driver-implementation": "^1"}, which is then provided via "provides": {"doctrine/orm-implementation":"1.0"} in the implementations. This has worked really well in doctrine/phpcr-odm:

https://github.com/doctrine/phpcr-odm/blob/1.4.4/composer.json#L21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants