Problem/Motivation
Currently, drupal/core-recommended is a metapackage that is created via a drupalorg infrastructure tool that builds it from the information in each composer.lock file of each version (tag) and branch of the Drupal git repository (drupal/drupal). This is done after a tag or branch is pushed (i.e. on every release, and every time a patch is committed).
In #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage, we discovered that if we attempt to do a composer create-project
test on the drupal/recommended-project
Composer Template Project, that said test will fail for any patch that updates the dependency versions in the drupal/drupal lock file. The reason for the failure is that the create-project
test attempts to combine the drupal/core
project, which it includes via a path repository so that it includes the changes from the patch being tested, and the drupal/core-recommended
project, which has all of the version constraints from the current HEAD of the base branch (e.g. 8.9.x-dev). This will work for any patch where the composer.lock file has the same dependencies as in the HEAD of the dev branch, but fails when these dependencies are updated.
The only way that a create-project
test could possibly work would be if the drupal/core-recommended
project were updated with the data from the composer.lock file in the current patch. Under the current design and implementation, this presents a chicken-and-egg problem.
Proposed resolution
We can resolve this problem by converting drupal/core-recommended
into a subtree split of drupal/drupal. We can update it every time a composer update
command runs, and give it a composer.json file that will cause the subtree split tool to update the split project on GitHub at the same time it updates drupal/core and the other split projects. (n.b. drupal/core-recommended
is a metapackage, and therefore consists only of a composer.json file.)
With drupal/core-recommended
being created as a subtree split, then and composer create-project
test that wishes to include drupal/core
as a path repository may also include drupal/core-recommended
as a path repository at the same time. In this way, these two projects will always have compatible information in their composer.lock / composer.json files, respectively, and the test will work.
We will also port over drupal/dev-dependencies
and drupal/pinned-dev-dependencies
to use the same technique so that we can be consistent in the way that the metapackages are generated for Drupal 8.8.x and later.
Finally, also for consistency, we will move the Composer hook that checks the Composer version to composer/Composer.php, because this hook is only needed for drupal/drupal; it is not needed by drupal/core users.
Steps to Test
1. Check out a branch of drupal/drupal, e.g. 8.9.x. If this isn't a fresh install, `rm -rf vendor`.
2. Run 'composer install'
3. Pick the most recent patch for the branch, e.g. #104 for 8.9.x, and apply it
4. Make a branch (`git checkout -b 3087626-104`) and then commit the results from (3)
5. Run: `COMPOSER_ROOT_VERSION=8.9.x-dev composer update --lock`
6. You should see an error message similar to the one below:
> Drupal\Composer\Composer::ensureBehatDriverVersions
Script Drupal\Composer\Composer::ensureBehatDriverVersions handling the post-update-cmd event terminated with an exception[RuntimeException]
Drupal requires behat/mink-selenium2-driver:1.3.x-dev in its composer.json
file, but it is pinned to dev-master in the composer.lock file.
This sometimes happens when Composer becomes confused. To fix:1. `git checkout -- composer.lock`, or otherwise reset to a known-good lock file.
2. `rm -rf vendor`
3. `composer install`
4. `COMPOSER_ROOT_VERSION=8.9.x-dev composer update ...` (where ... is
the update arguments you wish to run, e.g. --lock).
Follow the instructions and run the same update command again. It should work now.
7. Pick a package and try to update it, e.g. `COMPOSER_ROOT_VERSION=8.9.x-dev composer update doctrine/lexer`
8. Confirm that the package was updated. If it was updated, then you should also see the message:
Updated metapackage file composer/Metapackage/CoreRecommended/composer.json.
If you make a patch, ensure that the files above are included.
9. Run `git diff composer/Metapackage/CoreRecommended/composer.json` and confirm that the package that you updated was changed in the metapackage.
10. Discard your changes (e.g. `git checkout -- .`), because we do not actually want to make any updates at this time.
Optional: try again for 9.0.x. Be sure to update `COMPOSER_ROOT_VERSION=9.0.x` if you do. This branch could be harder to test, though, because the composer.lock is up-to-date, so it might be hard to do step 6. If you test both branches, make sure that you REMOVE YOUR `vendor` DIRECTORY COMPLETELY before switching branches and running `composer install` again.
Remaining tasks
Convert the package generator function that createsdrupal/core-recommended
into a post-update script indrupal/core
.- Modify the package generator so that it continues to creates tags and branches for Drupal 8.7.x and earlier, but does not create tags or branches for Drupal 8.8.x and later. drupalorg-infrastructure/package-generator#7
Update the documentation with instructions on how to deal with Composer updates when making packages.See https://www.drupal.org/node/3089540- Sit back and watch the existing subtree split tool automatically keep the 8.8.x versions of
drupal/core-recommended
,drupal/dev-dependencies
anddrupal/pinned-dev-dependencies
from the files we write into drupal/drupal.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
n/a
Comment | File | Size | Author |
---|---|---|---|
#118 | 3087626-118-8.8.x.patch | 50.92 KB | jibran |
#118 | 3087626-118-8.9.x.patch | 50.92 KB | jibran |
#118 | 3087626-118-9.0.x.patch | 51.3 KB | jibran |
Comments
Comment #2
Mile23To me this means that we should not have a drupal/core-recommended, but instead have trustworthy dependency constraints in drupal/core.
The problem with that is that we then have to do min/max testing on multiple platforms at some point in the CI/release process.
Comment #3
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThere is an existing long discussion of min/max testing vs. drupal/core-recommended in #3076600: Create drupal/core-recommended metapackage. My opinion has not changed, but if we are going to resume that conversation, we should do so on that issue, or make a follow-up issue, and reserve this thread for considerations on how drupal/core-recommended is built.
Comment #4
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere is a patch that converts the method of creating drupal/core-recommended to a post-update hook which is subsequently subtree-split out.
Comment #5
Mile23OK, so this patch updates composer/Metapackage/CoreRecommended/composer.json after updating anything with Composer.
This means that if you're a core developer, you might well see a changed file after
composer update
, and that you should commit it for your patch.I'd suggest that we also trigger this on project install as well, because it's possible to update, reset a few composer.json and lock files here and there, and then install, leaving the metapackage out of date.
I'd also suggest running
composer update --lock
so there's a start state for the metapackge, and adding that to the patch here.Comment #6
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAdding a message whenever the metapackage changes is a good idea.
I'm not sure that the use-case of updating after
composer install
is necessary. If the user modifies composer.json, then they should runcomposer update --lock
, which will update the metapackage.Also, the metapackage does not need a composer.lock file. It is never installed as a top-level project, so the composer.lock file is always ignored. Metapackages also can only have one file, their composer.json. Finally, if we did make a lock file for drupal/core-recommended, it would always have the same info as the composer.json file, which pins each dependency to a specific version.
Comment #7
MixologicOne thing that I wondered but never said out loud with the other metapackages, is whether or not they needed to include a README.md, or whether they are _required_ to have a LICENSE.txt. The former seems optional, and maybe gets in the way of a scaffolded readme, but I dont think you can have a subtree split, without a LICENSE.txt
Comment #8
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYou can write files like README.txt and LICENSE.txt into a metapackage, but when you `composer require` the metapackage you do not get any of these files; you only get the composer.json.
If we must have files, we can do the same thing we do with the legacy-scaffold-assets project: call it a metapackage informally, but in fact register it as a "project" instead of as a "metapackage".
Comment #9
Mile23composer update --lock
in the repo.Comment #10
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI'm ambivalent about #9. On the one hand, if you revert your composer.lock file, you should revert your metapackages. On the other hand, folks don't know to do that yet, and it's not a lot of overhead to regenerate the metapackage on every
composer install
. It just seems unnecessary to do it then, as the metapackage only changes oncomposer install
in edge cases.Comment #11
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdated patch - not running on any additional events at the moment, but it does print a warning whenever a metapackage changes.
Updated the code by porting from the existing Package Generator. No interdiff, because the implementation is completely different.
Comment #12
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedLet's run the tests too.
I will also mention that the test fixtures in #11 contain a full composer.lock from Drupal 8.8.x. If this is viewed as being too much to tuck away in an obscure test directory, we could always pare it down to contain just a few dependencies, and update the expected results in the tests to match.
Comment #14
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRemove some dead code, fix some code style problems, and fix a couple of broken tests.
Comment #15
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented... and run the tests.
Comment #16
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #17
MixologicOkay, I reviewed, screenshared with @greg.1.anderson and shuffled some things around.
Removed the defaults from the code - they now read in the existing composer.json's - This is under the assumption that 'codebase wide' composer.json scans happen and we dont really want folks to have to remember where to change additional things, because its in php instead of in json on the filesystem.
Also, I moved the lookup table for the pinned dev dependencies to be more like it is for the regular dependencies -> a couple of if statements with a TODO to remove later once upstream gives us something to move to.
Also found/removed several instances of dead code, as well as some places where earlier branch compatibilty stuff was coming in from the upstream package generator.
Minor docblock fixes too.
Comment #18
MixologicI had one question: wasnt sure about why we have drupal.core as a requirement of pinned dependencies.
Comment #19
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commenteddrupal/core is a requirement of pinned dependencies because if you care to strongly constrain your dependency versions, it's important that they are strongly constrained to something specific. It wouldn't make much sense, for example, if you constrained your drupal/core to ~8.7.0 and then your "pinned" dev dependencies floated up to ^8.8.
I think that only the tarball generator will use pinned dependencies, for the most part.
Comment #20
MixologicAh, sure. that makes sense.
Oops. I had mistakenly taken these out, then put them back in on second thought and missed this one.
Also, Im going to trim the fixutres - they dont need any of their requires in there, plus that'll make for a better test by proving theyre different.
Comment #21
MixologicAIght. added that pinned dev dependencies thing back in.
Made a couple other small changes: The 'no newline at end of file ' was probably going to make many other patch submitters annoyed, so I altered the trims to not check for newlines,
I stripped out some of the unnecessary stuff from the fixtures, and adjusted the output message to include the full relative path of 'core/Metapackage'.
Comment #23
Mixologiccoding standards and test fixture fixes to fix fixture test.
Comment #24
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI made this review on #21 w/out reading #23 (cross-post):
I'd just:
```
$composer['require'] = [
'drupal/core' => 'self.version',
];
```
No need to unset here, we replace on the next line.
Comment #25
MixologicYeah, I just blindly migrated the clearing out to the other two classes. Thats a better strategy.
Comment #26
MixologicComment #27
MixologicThe interdiff in #25 is still relevant
Comment #28
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI am +1 on #27, but cannot RTBC a patch I started.
n.b. Still only runs on post-update-cmd. If folks have trouble after reverting composer.lock, they can always run
composer update --lock
. That's my preference, anyway.Comment #29
jibranThis looks ready. Just a couple of minor points.
> 80 char. We need a follow-up to add composer dir to
core/phpcs.xml.dist
Pinning a version is the same thing as mentioning commit SHA so why can't we leave these a commit SHA?
Let's move the test to
Drupal\Test\Composer
dir and namespace as well.Comment #30
jibranI tired the composer script with #3088369: Update Drupal 9 to Symfony 4.4-dev works fine.
Ignore 2 in #29 it shows up as dev-master so changing this is fine.
Comment #31
Mile23Doing some manual testing....
Yields a change to the lock file and thus drupal/core-recommended, with a nice round-trip on remove that ends up with a reverted drupal/core-recommended.
Again changes dev dependencies and pinned dev dependencies, with a successful round trip on remove.
So we try this:
This updates the lock file:
So neither of these should make it into the metapackages, which they don't. However, now the lock file still contains the branch name.
We fix that:
And all is well, our lock file is not touched.
Now I edit core/composer.json to require crell/api-problem @stable and run the same command:
This adds the dependency and updates core-recommended.
Every time the metapackages are updated, I see this (with appropriate files listed):
Being able to tell people to run
composer update --lock
if they revert their lockfile is adequate, but not perfect.I'd RTBC except for #29. Also: I couldn't find where we exclude composer/ in phpcs.xml.dist.
Comment #32
jibranWe have
which means only
core
is checked.Comment #33
Mile23Derp. In a directory.
Comment #34
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#27.1: Wrapped. I'll write up a follow-up issue. Do we want to just include
../composer
as an additional path to sniff? This is a bit odd (just like the tests incore
that test code incomposer
), but duplicating the phpcs configuration seems like it would be even worse.#27.2: Version constraints such as
dev-master#sha
may only be placed in root-level composer.json files. If this form is used anywhere else, the #sha part is ignored, and the version is interpreted as simplydev-master
.#27.3: Moved.
Comment #35
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI added a follow-up issue #3088730: Include 'composer' directory in phpcs scans.
Comment #36
jibranThanks, for addressing the feedback.
Comment #37
MixologicSo I just realized that if we switch to this/backport it to 8.8.x, then the 8.9.x version of core is going to get the
wikimedia/composer-merge-plugin
back.The external package generator removed that, so, I think we should make sure it doesn't make its way back into the tarballs.
Comment #38
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedwikimedia/composer-merge-plugin is being removed here.
We want this removed in both the 8.8.x and 8.9.x versions of drupal/core-recommended.
We want to add wikimedia/composer-merge-plugin back in to the legacy tarball, but not the recommended tarball.
We will remove wikimedia/composer-merge-plugin completely in 9.x.
Comment #39
MixologicDoh. I was going off of my memory of having worked on the patch and not remembering that was there. My bad.
Comment #40
xjmSo this seems like a lot of overhead to maintain. Every time we update a dependency version, we have to change all of these things in all of these places?
Comment #41
jibran@xjm They will be changed automatically with the post-install and post-update scripts.
@greg.1.anderson @Mixologic this reminds me, do we need post install script to compliment
"post-update-cmd": "Drupal\\Composer\\Composer::generateMetapackages"
?Comment #42
MixologicIt only needs to run when composer.lock changes, so post-install isnt necessary.
Comment #43
alexpottI think we need a test to ensure that root composer.lock file is correct and the meta packages have been updated appropriately otherwise the committers will struggle to keep all this in sync.
Comment #44
MixologicEvery time you update a dependency, a message will tell you they've been updated, and that you should add them to your commit.
They're essentially far less fiddly composer.lock files, and may help us eventually dispose of having a lockfile in git, and all the conflicts and rerolls that come along with it.
Okay, so a test somewhat similar to
core/tests/Drupal/Tests/ComposerIntegrationTest.php->testComposerLockHash() ?
Thats a great idea. We should put the
"content-hash": "8ba406bd7f3522d51f0e55fe33e51aff",
from the composer.lock into the generated composer.json files and we can make the test compare them.Comment #45
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI'm +1 on #43, but I don't think we should copy the content hash. The test can just run the generator and see if the metapackages match.
Comment #46
Mile23This whole thing is supposed to get us around a corner: #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage
We can't yet add a build test of
composer create-project drupal/legacy-project
because that project template uses drupal/core-recommended instead of drupal/core. drupal/core-recommended currently exists outside of whatever patch you might have made to the dependencies, since it's generated after commits. Therefore we might have a false pass/false fail on a patch that changes the dependencies, or we won't know if there is a dependency problem within drupal/legacy-project until after the commit happens. This applies to the drupal/recommended-project template as well.The solution here is that instead of having it generated after commit, we generate it on Composer update, to be included in the patch. That way the metapackages are updated per patch. The downside is that there might be a couple extra files to deal with which are generated automatically.
See #31 for a walkthrough.
Note that this is all predicated on the need for a drupal/core-recommended so that we can lock down on a known-safe version of a given dependency.
A good test to add to this patch would be some of the commands from #31.
Comment #47
MixologicI assume #46 is in response to #40
But a clarifier:
The intention of the test in #44 is not to detect dependency problems. It's to detect a failure to add the correct metapackages to any submitted patch.
Comment #48
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI am wary about running `composer update` tests on the root of the system-under-test, and I'm not sure that the test would be valid on a copy unless we copied a lot of the source tree. We could perhaps do the latter type of testing in the Build Tests, and a create-project tests in the follow-on issue (#3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage).
Here, we should just verify that the metapackages are up-to-date with tests such as described in #43 - #45. If we do that, then we should be covered well enough. If `composer update` isn't working correctly, then the bad results will go into the patch, and we'll catch it in testing.
Comment #49
Mile23That's what I mean. I was thinking a test like this:
composer config repositories.empty path core/tests/Drupal/Tests/Composer/Plugin/Scaffold/fixtures/empty-fixture
composer require fixtures/empty-fixture
composer validate
.But this isn't a good test, because it doesn't demonstrate that we've added a dependency, since it skips path repos.
However..... Performing this test manually yields metapackage composer.json files without name, type, description, license, or conflict keys. So the generator should probably always generate those keys instead of relying on their being in place in the file.
So we go ahead and test the internet a little bit:
composer require something/something
composer validate
.composer require --dev
.This has the same missing keys problem as before, but also demonstrates that we get the desired outcome.
I'd write these tests, but then I can't review them. :-)
Comment #50
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI wrote it that way for testability, but that version was backed out in #27 because it reduced the size of the code.
I think we should do things like #49 as follow-on work when we do the Build Tests. I'll write the tests for #43 right now. We need to ro-sham-bo and decide of the initial metadata for the metapackages should be in code, or in-place in the metapackage target location.
Comment #51
MixologicRo-sham-bo says "yes, put the metadata generation back into code" My initial objections were worries about cross codebase edits of composer.json files being able to hit the metadata packages.
Except we dont want that, and with a test like #43 suggests, we'll end up with some fails if somebody does do that, so that will protect us.
I believe the slack plan is:
re -add the metapackage generation back in.
have a test that does comparisons between whats in the metapackages vs whats in composer.lock to ensure they're equivalent.
Comment #52
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's an updated patch that puts the metapackage metadata back into code, and provides a test to confirm that the metapackages have been updated along with the most recent update to the composer.lock file.
Comment #53
MixologicSolid ++!
hadn't seen this before. I was used to $root = realpath(__DIR__ . '/../../../../'); kind of stuff, but then I went searching core and it seems like theres a lot of dirname russian dolls, so, +1
nice. Me gusta
Comment #54
jibranLet's create a white list for exceptions so that we can keep track and add tests to make sure we don't forget to update these generators when we add/remove and exception. Thoughts?
Comment #55
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedIt is my hope that Drupal never again ships a stable release that contains a dependency required with a non-stable version constraint. It is also my hope that we eventually will get a stable tag for these two components, and will be able to simply remove these exceptions. I understand the circumstances that lead up to these components being included like this (and why we cannot just remove them now). If such an unusual set of circumstances happen to align again, then we can work on generalizing these exceptions as follow-on work. Hopefully it will never come to that.
Comment #56
MixologicYeah re #54 I worry that a whitelist will get added to. Lets not.
Comment #57
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis should also be backported to 8.8.x
Comment #58
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRe-rolled for 9.0.x.
Comment #60
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFixed some code style issues and added a TESTING.txt file.
Note: #58 inadvertently included the ProjectMessage plugin from another patch. This code is removed in this patch. The removal is not shown in the interdiff, though, as the removed code should not have been included and has no bearing on this issue.
Comment #61
MixologicComment #62
MixologicPatch already needed a reroll from another composer.lock update.
Also, Renamed
core/tests/Drupal/Tests/Composer/Metapackage/MatapackageUpdateTest.php
to MetapackageUpdateTest.php.Sneaky little vowel.
I did notice that for some reason, when rolling the patch, my composer.lock reverts from dev-master for the behat/mink-selenium2-driver to 1.3.x-dev version, which then causes the pinned-dev-dependencies to not get the wide net version we put in there:
Im not sure what part of the composer lock updating mechanism switches it from dev-master _back_ to 1.3.x-dev but it happens somehow.
Comment #64
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe reason that
behat/mink-selenium2-driver
switches to1.3.x-dev
is that the 1.3 branch exists there now, whereas previously, 1.3 was a branch alias for master the last time that project was updated in the lock file (ergo the previous dev-master result).We should add special checking for 1.3.x-dev in addition to dev-master, and also put in a similar check for 1.7.x-dev for behat/mink, in case they make a 1.7 branch for us there as well. (behat/mink will break all old Drupals if they change their branch alias from 1.7 to 1.8 without making the branch).
I'll make those adjustments.
Comment #65
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOh, wait, it's going the other way -- it was 1.3.x-dev and is now dev-master. I do not know why, but it probably has something to do with the circumstances in #64.
Comment #66
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis is odd:
After running ^^ there were no diffs in my composer.lock file, and behat/mink-selenium2-driver was dev-master in the lock file, as the package generator expects, so the metapackage was fixed up correctly.
Did you remember to remove
vendor
when switching branches? Composer can behave strangely if that's left around. Ah, and that's why I got some packages changing version when I ran composer update; they changed in the 9.0.x branch, and I did not run composer install before running composer update. Got it.Comment #67
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedPer discussion with @mixologic and @mradcliff:
Composer wants to float the version of behat/mink-selenium2-driver up to 1.3.x-dev, because there is a 1.3 branch in that project now. Previously that version was converted to dev-master (what's committed in the composer.lock file right now) because there was a branch alias from 1.3.x-dev to dev-master. That's gone now in the upstream project, but it persists in our lock file, resulting in inconsistent behavior.
I'm going to force behat/mink-selenium2-driver up to 1.3.x-dev in our lock file so that core contributors do not keep encountering this problem, which causes the metapackage to be generated incorrectly and results in a corresponding test failure -- and it's hard to know why that happened. Fixing this here will avoid that future pain.
Comment #68
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI would recommend crediting @mradcliff on this issue; I don't seem to have the permissions to do that.
Comment #69
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFixed per #67. Sorry if this caused any confusion, but I accidentally named the patch in #66 #68.
Note that now we are going to need to backport to 8.9.x; we won't be able to use the earlier patches. I'll do that after this is committed.
Comment #70
MixologicOkay. this is looking solid now, and will allow us to have the metapackages as part of core now, and thus they cannot get out of sync.
Comment #71
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere is the backport to 8.8.x. I understand that we want to label our patches with the lowest release they will be committed to now.
Comment #72
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#71 applies to the 8.9.x branch too, so I queued up another test for that.
Comment #73
xjmThanks @greg.1.anderson! For the most part that's true, but I left a nuance out of the tweet. :) Patches may be backported to 8.8.x at committer discretion but should be committed to 8.9.x before deciding that under policy. As per https://groups.drupal.org/node/535412:
So, this isn't guaranteed to go into 8.8.x (but there's probably a strong need/case for that based on what I heard from @larowlan or @webchick or someone; I don't know all the details).
Unrelated comment. I still see four statically coded places in the patch (in addition to the two already in HEAD in
composer.json
andcomposer.lock
) that have to track dependency versions, and no update to documentation or committer tooling or workflows to help maintain them. What's the plan for that? We're at least missing the docs gate here for it, and I'd like to know how those of us not involved with this initiative will be able to maintain this without having to grep core for a unique version number string to find the places.Comment #74
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@xjm: The composer.json files in the Metapackages directory are generated files. They are updated automatically by a Composer post-update command whenever composer.lock changes. Whenever the Metapackages are updated, you will see a message similar to this:
Regarding documentation, it would be swell if the drupal/drupal repository had documentation in markdown that caused pages to be published on drupal.org when the patch was committed. Since we don't have that, though, I am left in sort of a quandary about what to do with issues that alter behavior described in handbook pages. If we alter the handbook before the patch is committed, then existing users will be confused that the documentation does not match the behavior. If we wait until after the patch is committed, then the same problem exists on the other end. If you'd like me to update anything before this patch goes in (maybe with a "disclaimer: coming soon" prequel?), then I'd be glad to do so.
This patch really really needs to be backported to 8.8.x; however, I'm happy to follow whatever process for doing that is appropriate. We have a patch that works in both 8.9.x and 8.8.x; we can commit to 9.0.x and 8.9.x first, and then have whatever discussion is necessary to decide whether to put it into 8.8.x as well.
Bumping the priority here up to "critical", as this patch is blocking making Drush work with Drupal 9.
Comment #75
xjmTechnically, "blocked not-core-project" would be major/contributed project blocker, but I guess we kind of know already that broken Drush is a special case and will be perceived as a critical regression, so I guess critical's OK here.
Also, I'm not sure how wise it is to have tracked core files being automatically changed by a script, such that a script is patching core? Are we expecting committers to do this? Would it happen with just a
composer install
, which is the only thing that happens currently on core commit? And also, having committers expected to commit something that's different from what's in a patch is also error-prone and problematic. (All these things are specifics that should go in whatever docs, also.) :)I don't know where to document it, but it needs to go somewhere in the contributor guide and also in the committer handbook and committer tools if there's something committers have to do that's different from what we do now. Someone on a different issue pointed out that we utterly lack info about composer in the patch creation handbook page, so this would be added there at least.
One can always put a line in the handbook (or change record, release notes, etc.) like "Following [#1234567], [...]" to get around the chicken-and-egg problem. So yah, essentially "Disclaimer: Coming soon".
Comment #76
Mile23These changes would only happen when someone updates the lock file.
So it goes like this:
composer install
composer update something/something
git add my/work.php
git add composer/Metapackages
git commit -m 'bunch of work'
git diff 8.9.x > my.patch
The reason we do this is so that there's a path repo metapackage in core that represents the lock file. If we don't have that, we can't test building the project templates, per #3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage So this is how we test whether your patch breaks
composer create-project
.Comment #77
webchickSo would the instructions for patch authors / committers basically read:
"In the event that you are adding or changing Composer libraries/dependencies, you need to do this..."
[what you said]
"Otherwise, the standard process applies"
?
I think that's what we really need to hone in on in the docs (and even more ideally, something like https://github.com/alexpott/d8githooks would remind us if we forget). Patch authors and committers both have decades of "muscle memory" around their respective workflows.
Comment #78
Mile23Yah, definitely changes to workflows can be hard to remember. Like in #74, if needed, it tells you:
And those changed files will show up in
git status
and so forth.I have no doubt there will be other workflow changes as we continue with the composer stuff. This one stands out because we have to test the metapackages per patch, so it has to be in the patch.
But there's not really an extra step here, other than to be sure you add the changes to the metapackages if they've been changed. That could happen if you
composer update
anything. It's also possible that there won't be a change if youcomposer update
and it ends up with the same lock values.Comment #79
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@webchick: The muscle memory will be about the same. The difference is that if you run
composer.lock
, you might get modifications in a few more files in addition tocomposer.lock
. You just need to make sure that these modified files become part of your patch. If someone forgets, there's a test that will fail.We will update documentation to explain more fully.
Comment #80
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedPlaces to potentially document this behavior:
We will update one or more of these pages.
Comment #81
Mixologichttps://www.drupal.org/node/3089540 is a documentation page I started on that really gets into "As a core developer, I need to change the dependencies of drupal core"
This patch is a *very* small part of it, adds an additional step to be aware of (up to 4 generated files instead of 1), catches you with a test if you didn't see the output, and haven't read the docs.
The impact of these changes should be mostly minimal in that the *only* time it affects people is when they are updating core dependencies.
The git history of the lockfile shows that this is around 3-4 times a month.
Theres an absurd flurry of changes happening now because composer intitative tends to touch composer stuff, and 9.0.x has many,many major updates.
Comment #82
xjmSo https://www.drupal.org/node/3089540 looks great (thanks @Mixologic!) but I have a new reason to be concerned here:
This strikes me as... very bad?
Edit: Ryan tells me that is another evil side effect of #2912387: Stop using wikimedia/composer-merge-plugin and unrelated to this here issue. (Still... very bad...)
Comment #83
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis patch is moving us in a better direction. I think we should consider an upstream change to Composer to prevent it from ever upgrading packages from path repositories.
Comment #84
larowlanComing in late here, but is this analogous to the es6 vs generated files in Javascript?
In which case we have building tooling in the d8githooks that prevents commits that update one but not the other.
We already do a composer install as part of that, I guess it would need to validate that the meta-packages match?
Comment #85
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@larowlan: Yes, we have a test that verifies that the committed metapackage matches what the composer.lock generates. If someone forgets to commit one, then they will get a test failure.
Comment #86
alexpottI'm more than +1 though because this also neatly solves problems we've had we've keeping webflo/drupal-core-strict and the new drupal/core-recommended up-to-date without any more magic or githooks so that's fantastic.
env COMPOSER_ROOT_VERSION=9.0.x-dev composer update --lock
shows all the deps in the metapackages moving to Symfony 4 as expected - nice!I think this documentation could be improved to describe what a metapackage actually is.
typo... classes
This is odd documentation. The methods are just below - i think these docs should be on the respective method if there's anything here that's not covered.
I'm pondering the utility of this test given we have the implicit testing of MetapackageUpdateTest
This test will get more and more stale over time. I think we should drop it and all the associated fixtures.
composer/Metapackage/CoreRecommended
composer/Metapackage/DevDependencies
composer/Metapackage/PinnedDevDependencies
vs
composer/Metapackage/Generator
I think maybe metapackages should be inside some subfolder or maybe we should have
composer/Metapackages
andcomposer/MetapackageGenerator
env COMPOSER_ROOT_VERSION=8.9.x-dev composer update --lock
orenv COMPOSER_ROOT_VERSION=8.9.x-dev composer update drupal/core
I get the following changes:Which is quite unexpected.
If we are going to keep \Drupal\Tests\Composer\Metapackage\BuilderTest (I hope not) then we need to also address:
Needs docs
This is not used.
Comment #87
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented9. Remove your vendor directory, maybe? The result you are getting is caused by having a branch alias for 1.3.x-dev to dev-master for the behat/mink-selenium2-driver. This was happening to us before because that branch alias was in the composer.lock file. That one should have gone away when you applied the patch. I'll experiment and try to reproduce later.
We need to keep BuilderTest because that is the test that determines that the Metapackage Generator is producing correct results. The other test compares the output from the Metapackage Generator (at pre-commit time) with the output of the Metapackage Generator (at test time). That test only confirms that the Metapackage Generator was run. It does not determine that the code produced was correct.
It does not matter if the BuilderTest fixtures become stale, because there is no reason for the fixtures to be the same as the Drupal core contents. In fact, the reason the fixtures exist directly rather than using Drupal's own lockfile (as the other test does) is to avoid having to make continuous updates to the tests as Drupal changes. Maybe it's confusing that the fixtures are an old version of Drupal. This was handy during the initial development, as it was easy to see in the tests that the results looked like we expected them to. If it would be less confusing, though, we could use a fixture that did not look anything like Drupal at all.
I'll fix up the other requested changes later today. n.b. we have two versions of the patch currently, for 8.9.x and 9.0.x. I'll make update both versions when I make the other fixes.
Comment #88
alexpottCommitting a whole composer.lock file for Drupal as test fixture seems likely to cause confusion. It would be less confusing if the fixtures didn't exist as composer.lock and composer.json files at all. We could inline a tiny composer.json and lock in the test.
I also question whether
is really the case. Correctness for these metapackages can't really be determined by this either. A correct metapackage is totally determined by Drupal's current set of dependencies and bugs in this code are unlike to be found by only this test. If we break the generator both tests will fail. If we've got something wrong it's most likely that usage of the meta-package is what'll discover that - not either of these tests.
Comment #89
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYou are talking about correctness from the standpoint of a functional test. We will add functional tests for the metapackages as build tests in a follow-on issue (#3086644: LegacyProject composer templates wrongly reference 8.x + fix test coverage). I was talking about correctness from the standpoint of a unit test. Given that we have methods that take inputs, we can assert that the outputs match our expectations. The BuilderTest does this, whereas the other one does not. I think that we need the functional test, the unit test and the test that checks to see if the metapackage generator runs, as they all perform useful and important functions.
I will simplify the fixtures for the BuilderTest and inline them. It will only take a few packages to prove correctness.
Comment #90
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOK here's an updated patch addressing #86.
1. Thanks!
2. I made the 8.9.x / 8.8.x version first; I will also re-roll for 9.0.x while the tests run here.
3. Thanks!
4. I added a README to the Metapackage directory, and referenced it from the interface comment.
5. Fixed.
6. Removed.
7. Simplified the unit test to use a minimal composer.lock file inlined in the tests.
8. Reorganized as suggested
9. This can happen if you have a vendor directory with the old behat/mink-selenium2-driver installed. I added a safety-check with instructions on what to do if your dependencies are out-of-date and causing problems.
1. Rewrote the way this class worked, adding docblock comments to all surviving methods. This also included the removal of the composer.lock fixture that was being used by the unit tests.
2. Removed.
Comment #91
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the 9.0.x version of the patch.
Comment #94
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedCode style fixes and a unit-test fix.
Comment #95
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedArgh. Run the test.
Comment #96
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis has an additional bugfix over #94.
The error message for a bad selenium2 driver now reads:
Previously,
COMPOSER_ROOT_VERSION=8.9.x-dev
was rendered as8.9.0-dev
instead of8.9.x-dev
(fixed above).Also switching to 8.9.x branch here; will post a 9.0.x equivalent next.
Comment #97
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the patch for #94 (9.0.x) updated with the bugfix from #96.
Comment #98
jibranSorry one minor thing.
Let's add a quick test for this method.
Comment #99
alexpottThis test is
notway more manageable - @greg.1.anderson thank you. [EDIT: the not should not have been here - sorry]Missing @return docs.
$referenceName can be removed. It's not used.
Comment #100
jibranAfter discussing with @greg.1.anderson I think we can ignore my feedback from #98.
Comment #101
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented1. Rerolled
2. Thanks!
3. Rounded out the missing part of the docblock
4. Removed unused fixture value
Comment #102
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the patch for 8.9.x and 8.8.x.
Comment #103
Mile23The 8.8.x patch in #102 doesn't apply, only because of the content hash in the lock file. That could probably be fixed on commit, but how about an 8.9.x patch, commit, backport.
We should probably ask a maintainer which branch to RTBC first and so forth.
Also this CS error, tho it's kind of a nit:
There are also a ton of CS errors from CSS and JS, but they're unrelated.
Comment #104
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOK, I'm not sure that I have the most up-to-date understanding of how Core development is supposed to happen, but I think:
Along those lines, here's a fix on the 8.9.x branch for the CS error mentioned in #103. I also added a "steps to test" in the issue summary in case that's helpful for anyone once the tests come back green.
Comment #105
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAnd now, keeping "Version" ad "8.9.x-dev", here is the 9.0.x version of the patch, which I presume is committed first. Actually I think both 9.0.x and 8.9.x will be committed at about the same time, so I don't know if order is relevant.
No 8.8.x patch yet; I will backport once these are committed.
Comment #106
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFix typo: ad-hoc test instructions are for 8.9.x-dev.
Comment #107
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRecommend a more recent patch in the ad-hoc instructions.
Comment #108
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedIt's important to remove the `vendor` directory if you've applied this patch before (vendor must be clean)
Comment #109
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedBe more explicit about the error message you see in step 5.
Comment #110
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdate the ad-hoc testing instructions; user must create a branch and commit in order to use 'git diff'
Comment #111
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFix numbers in instructions list
Comment #112
Mile23Can confirm that both #104 and #105 pass the manual test in the IS. That is, the 8.9.x and the 9.0.x versions work.
They demonstrate that we're catching the errant lock file changes to behat versions, and that the instructions they give actually fix the problem.
We'll need a backport to 8.8.x, per the tag.
Comment #113
jibranRTBC+1, nice work @greg.1.anderson. Just on nit not a commit blocker.
Maybe link the @todo to https://www.drupal.org/node/3078671
Comment #114
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI updated #3078671: Pin behat/mink and behat/mink-selenium2-driver to use resolvable release to contain another task to remove the test and script that will not be necessary when that issue is done.
Comment #115
alexpottSaving issue credit
Comment #116
alexpottThe 8.9.x in #104 contains unrelated change. I think we need to commit this to 9.0.x and 8.9.x at the same time.
Some very small nits.
Should be a fully qualified class name with a leading \
FQCN too...
Comment #117
jibranAddressing #116
Comment #118
jibranSetting it RTBC as it is just a reroll/rebase and doc fixes. Uploaded three patches for committer's connivance.
Comment #119
jibranUploading the diff between 8.x and 9.x patches to make sure that code is the same and the only difference is composer files.
Comment #120
alexpottCommitted 60b4730 and pushed to 9.0.x. Thanks!
Committed 55580b7 and pushed to 8.9.x. Thanks!
Ands asked @catch about backporting to 8.8.x as that needs RM approval.
Comment #123
alexpottDiscussed with @catch who is concerned about the current changes to the way we use composer making things that used to be easy harder but agrees that we should put this in 8.8.x for as "we have to push forward it could just be messy for a while".
Committed 770237b and pushed to 8.8.x. Thanks!
Comment #125
jibranRemoving the tag.
Comment #126
mondrakeI don't know if this is related, but this was filed in Imagemagick's issue queue: #3090559: Composer install fails with "Package drupal/file_mdm_exif-1.1.0.0 must have a source or dist specified". Any comment or review appreciated.
Comment #128
cilefen CreditAttribution: cilefen as a volunteer commented