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

Commit composer.lock and platform version #66

Closed
wants to merge 1 commit into from
Closed

Commit composer.lock and platform version #66

wants to merge 1 commit into from

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jul 6, 2018

By locking the platform version we encourage a good practice which prevents people from messing up with a prod server that has a different version than the one on the dev's machine that ran composer up.

By committing the composer.lock file, we optimize a lot the installation process.
Here is a comparison of a create-project with/without the lock file:

Without lock With lock
Nb of files downloaded 167 40
Peak Memory 274MB 120MB
Duration on regular network 19s 12s

The difference of duration can be much worse on slower networks.

Process-wise, this will require us to ship a new lock file with each new Symfony version. This feels like something we can automate in the releasing process, and will give us the opportunity to synchronize skeleton's version numbers with Symfony'.

Here is a Blackfire comparison with the website-skeleton:

image

https://blackfire.io/profiles/compare/42c40cf5-46d0-40df-acf3-94e929482889/graph

This illustrates how much this will improve the experience of installing Symfony for newcomers.

@linaori
Copy link

linaori commented Jul 6, 2018

While it's a very interesting approach, I don't like the idea of being locked into a specific version (say 7.0.8) if I'm running 7.1 or 7.2, it might lead to receiving "outdated" versions of the dependencies. I'm not exactly sure how else to do this neatly though. Would it help to specify the minimum patch versions in the requires instead? Perhaps a --super-optimized flag?

It seems like the original issue is somewhat related to this: composer/composer#7273

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 6, 2018

This PR is for branch 3.4.
We will bump the platform to 7.1 on 4.0.
Then, of course, this file is just the starting point for projects and will be edited by users.
It's still useful to encourage people to actually use that setting.

@Toflar
Copy link

Toflar commented Jul 6, 2018

I don't think this is a good idea. When I run create-project now, I will actually get wrong dependencies. Your composer.lock will give me dependencies for PHP 7.0.8 but I have 7.2.7 so I would get different library version combinations. If you commit that I have to run composer create-project, adjust the composer.json and then run composer update again. Nothing about this process helps to save memory, it just makes things more complicated.

I agree that configuring the PHP version in the platform config is a good idea to make sure it matches the production system but you have to do that in your local project not in the skeleton.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 6, 2018

@Toflar what matters is the time to first success. You will not have "wrong" dependencies, you will have dependencies that work on 7.2 also. And the skeleton is just the starting point, which we lock here to make it stable and fast to bootstrap.
I think that will improve the experience of installing Symfony a lot.

@linaori
Copy link

linaori commented Jul 6, 2018

If I were to use phpunit as dependency (which it is, because my tests depend on the public API of it), I would get an old version of phpunit though, @Toflar does have a valid point on that: https://github.com/sebastianbergmann/phpunit/blob/c1a87c0a4d9457c8ca051ef8b0b8aac0339207a9/composer.json#L24

For developers less experienced with package management/composer or development in general, this might cause more problems than it solves in performance, as they will not receive the dependencies they'd expect.

@Toflar
Copy link

Toflar commented Jul 6, 2018

No, that's not true. Think about a dependency that defines requires like so:

"require": {
    "php": ">7.0 <7.2",
}

because it uses something that explicitly only works for PHP 7.0.* and 7.1.*. I would get the dependency even though it fails on my system when I have PHP 7.2.*.
Sure, this example is probably very rare but that's what constraints are about. Any combination is possible.

you will have dependencies that work on 7.2 also

This is an assumption and you don't know if that's true. Using 5.6 and 7.* would have probably served as a better example. Also, you would need to update your composer.lock all the time when any of the dependencies releases a new - say - security relevant version. Otherwise this skeleton would provide a set of packages that contains packages with security issues.

It's not a good idea to commit the composer.lock for a skeleton. It will make it faster to bootstrap, yes but at the cost of probably providing a combination that doesn't work for your system. This approach will very likely cause issues at some point.

Regarding the platform config. I think it would be better if Flex checked the composer.json and showed a warning/recommendation to add the PHP version there to ensure you're not installing dependencies that do not match the target platform.

@nicolas-grekas
Copy link
Member Author

That's a skeleton. Dependencies are not random but minimal. This issue doesn't exist with any deps of the skeleton. About platform, putting a recommendation in Flex will add complexity for something that is unrelated to its business. Then, if people don't know how to deal with deps, its even more important to lock the platform, because they will just mess up with local vs remove versions of PHP.

Which means I'm still pushing to commit this lock file.

@Toflar
Copy link

Toflar commented Jul 6, 2018

For newcomers that means it all installs faster now but it probably installs packages with security issues or incompatibilities.

For experienced devs it means you have to run create-project, adjust the composer.json to match your platform and then run update.

Don't get me wrong, I love your efforts on trying to make this experience better but what I think it actually does is that it prefers performance over integrity and that's not a good decision. But that's my 2c, let's see what other devs think 😄

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 6, 2018

probably installs packages with security issues or incompatibilities.

impossible: we manage the lock file and we release new version of the skeleton as part of each new version of Symfony. Again, that's not random deps, but deps of the skeleton.

For experienced devs it means you have to run [...] update

That's a skeleton. You WILL have to use composer very soon. Nothing new here.

@Toflar
Copy link

Toflar commented Jul 6, 2018

That's a skeleton. You WILL have to use composer very soon. Nothing new here.

Yeah but what's the point then? I will have to use composer update very soon anyway. So either it takes 19s and uses up 300MB of memory during create-project or it takes 19s and uses up 300MB of memory 5 minutes later when I have to run composer update 😄

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 6, 2018

See above:

time to first success

That is a CRITICAL feature.

@Toflar
Copy link

Toflar commented Jul 6, 2018

Okay, I see. I can't argue with that. That's the only benefit and I would choose integrity over that any day. But well, I tried... 😄

@linaori
Copy link

linaori commented Jul 6, 2018

What would the performance be in the following scenarios if you have this composer.json?

  • The current situation without lock file for the first process that installs the packages
  • The new situation for the first time running composer update
    "require": {
        "php": "^7.0.8",
        "ext-ctype": "*",
        "ext-iconv": "*",
        "symfony/console": "^3.4.12",
        "symfony/flex": "^1.0.80",
        "symfony/framework-bundle": "^3.4.12",
        "symfony/lts": "^3",
        "symfony/yaml": "^3.4.12"
    },
    "require-dev": {
        "symfony/dotenv": "^3.4.12"
},

@maxhelias
Copy link

I have no problem with it, because after starting a project the composer.json file is often modified.
If this feature can convince users and guide them to good practice, then why not

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 6, 2018

@iltar I'm not 100% sure to understood your question, but if you're wondering how performance would change by narrowing the version windows (^3.4.12), the answer is not at all: solving the dependencies is not the resource consuming part. What is resource intensive is 1. downloading the json repositories from packagist 2. keeping them in memory. And this has to be done whatever the version windows are.

@aschempp
Copy link

aschempp commented Jul 6, 2018

probably installs packages with security issues or incompatibilities.

impossible: we manage the lock file and we release new version of the skeleton as part of each new version of Symfony. Again, that's not random deps, but deps of the skeleton.

Well that means you won't get any updated packages when no new Symfony version is released. There could very well be a dependency of Symfony that does have a security issue and you're not getting it.

On the same page, you won't get Doctrine DBAL 2.7, because it only works with PHP 7.1. So the Doctrine version you get won't support MariaDB 10.2

@anyt
Copy link

anyt commented Jul 6, 2018

Not sure this will work but, did you consider to use some composer events, like pre-package-install to make this option configurable?

For example you can ask user what version to set to composer.json before resolving dependencies:

  • use pre-defined version
  • use current system version if it fulfils minimum-requirements
  • set custom one, that developer can get from the production server, for example

Or at least display a notice to developers so they will be aware that installed dependencies are not optimal for their environment, and how to change it.

@javiereguiluz
Copy link
Member

What we did to avoid this issue in the deprecated Symfony Installer was to automatically remove the config.platform option from composer.json after the project was created (see https://github.com/symfony/symfony-installer/blob/ff32aacce264076f26b6924e5dde0ff9ffb36773/src/Symfony/Installer/Manager/ComposerManager.php#L18). Maybe we can do the same here?

@nicolas-grekas
Copy link
Member Author

@javiereguiluz I didn't know about this, thanks for the reminder.

I'm mixed on this: on one side locking the platform prevents installing e.g. Doctrine DBAL 2.7 as @aschempp noticed, on the other side removing config.platform feels like a missed opportunity to set the option and encourage a best practice that prevents some nasty mistakes (we don't want to make Flex interactive so we don't want to go down with @anyt proposals.)

Still, OK to remove config.platform on my side.

@javiereguiluz
Copy link
Member

By the way, I forgot to say that I like this a lot!! I agree with Nico that the first installation time is a critical metric. A minor question related to this: should we remove ext-ctype and ext-iconv from composer.json and replace them by Symfony's related polyfills?

PS: I also hope that some day the entire PHP community organizes a joint effort to massively improve Composer's performance. It's sad that Yarn takes a few seconds to do anything and Composer takes so long even for small apps. Of course I'm not complaining about Composer's maintainers. Thanks to them for everything they do 🙏

@fabpot
Copy link
Member

fabpot commented Jul 6, 2018

First of all, making things faster is a laudable goal. I like that a lot. Now, how to achieve that (without re-introducing an installer)?

Several issues raised in the PR.

The first issue is about deps that are not part of "symfony/symfony". They might be updated between two releases, and the lock won't be updated (that was also the case with the Standard Edition). This issue does not really exist for the bare skeleton (this repo) as we only depend on a sub-part of symfony/symfony (+ the polyfill), but it definitely exists for the website skeleton (which depends on Twig, Swiftmailer, Doctrine, ...).

The second issue is if you would have had more recent/different dependencies because your version of PHP is different locally (we compute the lock with 7.0.8, but you have PHP 7.2.4). I think this is marginal and not something we want to take into account. We can reconsider if that's really a problem later on. What's important is to not have the platform hardcoded in the composer.json file.

With that in mind, I'd like to propose something different from this PR:

  • We don't change the composer.json file and we only commit the composer.lock file (and we make sure that the right version of PHP is used to generate it like 7.0.8 for Symfony 3.4).

  • We setup a cron that each hour tries to update the composer.lock. If there are some changes, it commits them and tag a new version (for Symfony 4.1 for instance, the next version would be 4.1.13.1, and if a third-party dep is updated before 4.1.14, we would release 4.1.13.2).

By using the cron feature of Travis (where we can nicely get all the PHP versions we need), that should be completely automated.

That way, people creating a project will always have the latest versions of all deps, including transitive ones. And a simple composer update gives you whatever updates depending on your local PHP version. Huge gains without too many drawbacks

WDYT?

@aschempp
Copy link

aschempp commented Jul 6, 2018

That way, people creating a project will always have the latest versions of all deps, including transitive ones.

That assumes they do run with the outdated PHP version (like Doctrine ORM 2.7 does not …)

@Toflar
Copy link

Toflar commented Jul 6, 2018

It's sad that Yarn takes a few seconds to do anything and Composer takes so long even for small apps.

This is comparing apples and oranges. PHP needs a SAT solver because you can use one class only ever once. Javascript doesn't because you can include the same modules in multiple versions if you work with modules 😄 Optimizing it in the current state is really challenging, I've tried for many, many hours...but this is a discussion for the composer/composer repository. I just wanted to make sure we do not compare PHP's package manager with npm or yarn in this issue 😊

@javiereguiluz
Copy link
Member

javiereguiluz commented Jul 6, 2018

@Toflar I don't want to go off-topic too much, but just to give you some more info. Executing composer create symfony/skeleton on my machine:

  • Called strtolower() 429,669 times
  • Made 72 non-parallel HTTP requests to packagist.org (compare that to Flex, which makes 1 HTTP request)
  • Called VersionParser::parseConstraints() 117,076 times
  • Constructed 117,075 Composer\Package\Link() objects
  • Garbage collector took 7.47 seconds (12% of total)
  • The 139 calls to json_decode() took 310 MB of memory
  • etc.

It's only a guess ... but there could be massive opportunities to improve performance here at all levels: I/O, CPU, memory, etc.

@stof
Copy link
Member

stof commented Jul 6, 2018

@javiereguiluz and all these should be discussed in composer/composer.

@nicolas-grekas
Copy link
Member Author

I like that plan @fabpot :)

Here is a first step: https://github.com/symfony/skeleton/tree/composer-update
This branch contains a .travis.yml that is now setup to run daily (the max frequency on Travis.)
Here is an example build: https://travis-ci.org/symfony/skeleton/builds/400979967

This computes the next version number by incrementing its 4th part, prefixed by the last tag from https://symfony.com/versions.json

We now need to make it push the update + tag it somehow.

@nicolas-grekas
Copy link
Member Author

First steps ready, Travi now creates pull requests automatically with updated composer.lock files.
Script is https://github.com/symfony/skeleton/tree/composer-update
See recent PRs. Next steps is having a bot that watches for them and merges + tags.

@nicolas-grekas
Copy link
Member Author

A minor question related to this: should we remove ext-ctype and ext-iconv from composer.json and replace them by Symfony's related polyfills?

I didn't answer this: the answer is no. ctype and iconv are always enabled, except on maybe on Alpine & custom builds. We really want ppl to have a great experience from start, so that even ppl that do custom things have great perf.

So the current situation is by design and desired.

@stof
Copy link
Member

stof commented Jul 10, 2018

well, for ctype, it is also related to the fact that the ctype polyfill is quite new. So this might be reconsidered.

But I agree about not using the iconv polyfill (thus, it is huge)

@teohhanhui
Copy link
Contributor

teohhanhui commented Jul 13, 2018

This computes the next version number by incrementing its 4th part

That's not semver-compliant, as I've raised in #70

Why not increment the patch version? It's the right thing to do.

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

Successfully merging this pull request may close these issues.

None yet

10 participants