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

add --prefer-lowest and --prefer-stable to update command #3450

Merged
merged 3 commits into from
Dec 14, 2014

Conversation

nicolas-grekas
Copy link
Contributor

Run composer update --prefer-lowest-stable and get all the deps installed to their lowest (pretended) supported version

Fixes #3440
Fixes #1902

@stof
Copy link
Contributor

stof commented Nov 21, 2014

@naderman there is a protected method selectNewestPackages in the DefaultPolicy class, but it looks unused. What is its goal ?

@naderman
Copy link
Member

@stof used to be a helper method for selectPreferedPackages, I'll delete it as it's no longer used.

@naderman
Copy link
Member

5333017

@nicolas-grekas
Copy link
Contributor Author

Please tell me if I should do anything before merge

@stof
Copy link
Contributor

stof commented Nov 21, 2014

hmm, adding an integration test covering this case to ensure it works ?

@naderman
Copy link
Member

Only thing I wonder is if we should have a regular prefer lowest without prefer stable? /cc @Seldaek @stof

@@ -697,10 +698,14 @@ private function createPolicy()
// old lock file without prefer stable will return null
// so in this case we use the composer.json info
if (null === $preferStable) {
$preferStable = $this->package->getPreferStable();
if (getenv('COMPOSER_PREFER_LOWEST_STABLE')) {
$preferStable = $preferLowest = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure about $preferStable being true ? this would prefer a 2.4.0 over a 2.4.0-beta release, while 2.4.0 is lowest.
I think it should rather force $preferStable to false, so that only the version ordering is used to find the selected version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's what I meant, currently it's called LOWEST_STABLE so that makes sense, but maybe we should support both LOWEST and LOWEST_STABLE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I tried without prefer-stable, and I went back in -rc1 or -alpha state, which is certainly not what I'll ever want to test. So, if the only use case of this is testing the lowest version you pretend to support, I'm not sure that anyone will go up to supporting alpha nor even beta versions.

@nicolas-grekas
Copy link
Contributor Author

I just added a small unit test. For the integration test, there is none for other ENV vars...

@naderman
Copy link
Member

well other env vars have a config equivalent, this does not?

@naderman
Copy link
Member

This should probably also somehow use the new getComposerEnv() to avoid accidentally influencing test behavior with environment variables set in the process. https://github.com/composer/composer/blob/master/src/Composer/Config.php#L306

@nicolas-grekas
Copy link
Contributor Author

Rebased to use config->getComposerEnv().
There is no corresponding config option because I followed comments on #3440,
please tell me if I should update.

@naderman
Copy link
Member

Yes I didn't mean to ask for one, just meant that's why there probably aren't env tests for the others?

@nicolas-grekas
Copy link
Contributor Author

I added a line in the travis-ci matrix so that composer itself is tested with its lowest deps.
That made me fix the composer.json.

@nicolas-grekas nicolas-grekas force-pushed the prefer-lowest-stable branch 8 times, most recently from fb0b747 to 0dafd11 Compare November 22, 2014 20:15
@nicolas-grekas nicolas-grekas changed the title add COMPOSER_PREFER_LOWEST_STABLE add --prefer-lowest-stable to update command Nov 22, 2014
@nicolas-grekas
Copy link
Contributor Author

In fact, while trying to correctly test composer with the new flag, I've been hit by the same problem as in #3448 So, I replaced the env var by an option on the update command.
To make tests pass with the new line in the .travis file, I also had to remove phpunit from require-dev, because in fact, your tests relies on an implicit version of phpunit's deps.
Tests are green again now.

@naderman
Copy link
Member

Err I understood all of that except why you had to remove phpunit from require-dev?

@nicolas-grekas
Copy link
Contributor Author

It's because tests do not pass: phpunit itself is broken when setuped with --prefer-lowest-stable

@naderman
Copy link
Member

Hm I see, but shouldn't we find another solution to that then than remove the dependency on phpunit which we kind of need to test locally?

@nicolas-grekas
Copy link
Contributor Author

So, spending one more hour on this, I still don't see any use case for specifying "prefer-lowest" but not "prefer-stable" together. In fact, and because I fail to imagine a use case, I fear people will start using "prefer-lowest XOR prefer-stable", and you'll get issues and increase support on your side.

Still, I added a commit that splits --prefer-lowest and --prefer-stable so everybody is happy :)

Really, after spending time on it, I see a flaw in your reasoning @naderman that may explain our respective pov: when someone sets @dev to a requirement, it never meant that the very first commit in the git history of that package is supported - but the exact opposite: only the very latest commit is supported.
This is very different from a "~2.3" which states explicitly that "2.3.0 and more" is supported.

Did you checkout my branch and run it against e.g. composer and/or symfony? If not, then you really should. That's the experimental argument that made me understand and have my current way of thinking.

I'd be happy to remove this last commit if you come to the same conclusion as mine. Or you can merge as is now if you prefer.

@dbu
Copy link
Contributor

dbu commented Dec 2, 2014

if my only dependency is "@dev" or something equally funny, then i can
install with any version. if i use some library with "
@dev", but my
project asks for "0.1.*@dev" i will end up with some old version (assume
there was never a stable 0.1 release for example.

@nicolas-grekas nicolas-grekas changed the title add --prefer-lowest-stable to update command add --prefer-lowest and --prefer-stable to update command Dec 13, 2014
@Seldaek
Copy link
Member

Seldaek commented Dec 13, 2014

@nicolas-grekas could you give an example of a package where it didn't do the right thing for you? Because even with @dev it wouldn't install the oldest commit since it has no knowledge of all commits, if a library has: 1.0, 1.1, 1.2.x-dev (master) as versions, the lowest, even with @dev, is 1.0.

@nicolas-grekas
Copy link
Contributor Author

Of course: any Symfony component with dependencies.
Here is for DependencyInjection when adding --prefer-stable after a first run with only --prefer-lowest:

> composer update --prefer-lowest --prefer-stable
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Updating symfony/expression-language (v2.4.0-BETA1 => v2.4.0)
    Checking out 54ee1629680bf4313412cef284c6b19fd64c7a29

@Seldaek
Copy link
Member

Seldaek commented Dec 13, 2014

Yes that makes sense, you use the first beta available as that is what a ~2.4 requirement specifies (any 2.4..). It may not be what everyone wants but strictly speaking that's what prefer-lowest should do IMO. If you use it you get what you ask for. But it probably makes sense to combine with --prefer-stable to most people :)

@nicolas-grekas
Copy link
Contributor Author

Cool, then we can merge as is?

@@ -700,10 +703,11 @@ private function createPolicy()
// old lock file without prefer stable will return null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding prefer-lowest support for the lock file? I understand it's probably not a common use case or none at all but for completeness and to avoid bad surprises it should be persisted to the lock. See a few lines above..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, not sure to understand... Can you drive me a bit more please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, basically the same as https://github.com/composer/composer/pull/3101/files + nicolas-grekas@4bd748b which fixed some mistakes :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except preferLowest isn't on the root $package so you'll have to pass it in to the locker directly I guess.

@Seldaek
Copy link
Member

Seldaek commented Dec 13, 2014

Yes except for the lock support sorry I missed that last time :)

@dbu
Copy link
Contributor

dbu commented Dec 13, 2014

Yes that makes sense, you use the first beta available as that is what a ~2.4 requirement specifies (any 2.4..). It may not be what everyone wants but strictly speaking that's what prefer-lowest should do IMO. If you use it you get what you ask for. But it probably makes sense to combine with --prefer-stable to most people :)

If my bundle claims to work with 2.4.0-alpha1 or later, by using ~2.4 then it should be tested with that. so i notice that i should have restricted that to whatever version i am actually compatible with.

@Seldaek
Copy link
Member

Seldaek commented Dec 13, 2014

@dbu I agree on principle but in effect nobody is likely to use your bundle with 2.4.0-alpha1 once a 2.4.0 stable is out, but ~2.4 will allow the alpha still, unless you do ~2.4-stable. Under normal conditions composer will take the stable if it's there anyway so I would say for "compat testing" --prefer-stable --prefer-lowest sounds reasonable (you'll catch problems but not waste time debugging issues occuring only with alpha/beta versions of your deps).

@dbu
Copy link
Contributor

dbu commented Dec 13, 2014

okay. did not want to argue against the combination of --prefer-stable and --prefer-lowest, only that --prefer-lowest alone should install an alpha or beta.

@Seldaek
Copy link
Member

Seldaek commented Dec 13, 2014

Ah ok yes for sure :)

@nicolas-grekas
Copy link
Contributor Author

prefer-lowest is now persisted in composer.lock

@alcohol
Copy link
Member

alcohol commented Dec 13, 2014

Can you be sure to update doc/03-cli.md and doc/04-schema.md?

@nicolas-grekas
Copy link
Contributor Author

CLI doc updated (but there is no schema change for composer.json)

@Seldaek
Copy link
Member

Seldaek commented Dec 14, 2014

Great, looks very complete now, thanks @nicolas-grekas!

Seldaek added a commit that referenced this pull request Dec 14, 2014
add --prefer-lowest and --prefer-stable to update command
@Seldaek Seldaek merged commit 901fd83 into composer:master Dec 14, 2014
@nicolas-grekas nicolas-grekas deleted the prefer-lowest-stable branch December 14, 2014 14:08
fabpot added a commit to symfony/symfony that referenced this pull request Dec 15, 2014
…s-grekas)

This PR was squashed before being merged into the 2.3 branch (closes #12542).

Discussion
----------

Test components using their lowest possible deps

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Once composer/composer#3450 is merged, we'll see if our deps are correct, and ensure that is kept over time by testing each component with its lowest possible deps.

Commits
-------

25fef27 Test components using their lowest possible deps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants