Meeting will happen in #d10readiness on drupal.slack.com.
| Gábor Hojtsy (he/him) |
Raised by @Spokje, quoting:We decide we need to manually test any core module before we commit the actual deletion issue. For core modules we already deleted we also have to manually do testing.Currently we haven’t decided how to test, so we really can’t guide people on how to do this.Since the range of modules and their functionality can be huge, I think we can/should keep the testing instructions rather “generalized”, so we can add them to the procedure-documentation and also create issues in the d.o. queue for each module so people can start working on those.Since the working of the module in (now) 9.x is tested by core tests and the working of the contrib incarnation of the core module is tested by the core-tests-moved-with-the-module in (now) 10.0.x, I think we only need to test the main thing that we can’t test with TestBot: Having the core module enabled, updating to (now) 10.0.x and composer-requiring the contrib incarnation.Would something general like instructions on the above procedure and testing that the module’s functionality keeps the same and no module content (think for example feeds in case of the aggregator module) is lost, be enough or do we need to be more specific? |
| xjm |
Skimmed, but I mostly agree. Specific things to test per-module can be added per-issue, ideally by someone who knows the functionality of the module |
| xjm |
This is one of the most important things we can do right now |
| xjm |
We'll also want to repeat the testing after we commit the actual removal from 10.0.x HEAD |
| Björn Brala (bbrala) |
Generalizing that is kinda hard. Although something along the lines of a test of the general capabilities of the module in Old then the same in new would pretty much be al you can say.What might be helpfull is that posting of the testing is just a set of steps that the tester did. This would make it possible to see if there are perhaps known holes by others and minimize duplicated efforts. |
| xjm |
Exactly, the most important thing is to document what you tested so the next reviewer can know what they don't need to repeat |
| Björn Brala (bbrala) |
Hmm didn't notice your posts @xjm , on mobile :sweat_smile: |
| xjm |
The steps are roughly "Install on Drupal 9.3, add some data, update to 9.4.x HEAD. There should have been an issue filed this week for testing related to Composer -- looking to see if it got filed. |
| xjm |
Of course Jira goes down right as I go to look for the issue link |
| xjm |
pause while I search the queue |
| Björn Brala (bbrala) |
:sweat_smile: |
| Björn Brala (bbrala) |
You got onprem? |
| Björn Brala (bbrala) |
That beast is a hassle if so :sweat_smile: |
| xjm |
#3299191: Ensure that modules moved to contrib in 10.0.x are installed as dependencies of other contrib modules |
| Björn Brala (bbrala) |
Hmm |
| Björn Brala (bbrala) |
Perhaps I could just publish a simple module with a major version for each of the coming deprecation. |
| Björn Brala (bbrala) |
That way you can include that module with the proper major and easily test if it works. |
| xjm |
@Björn Brala (bbrala) Composer can install @dev--I test it all the time with core |
| xjm |
The commands are on release nodes even :smile: |
| Björn Brala (bbrala) |
Yeah but you would want one dependency per module |
| Björn Brala (bbrala) |
You could dev-hal as a branch |
| xjm |
@Björn Brala (bbrala) Sorry, the issue I linked is for later, after we commit the actual deprecation/removals |
| Björn Brala (bbrala) |
But just making a major release is less complicated for possible contributors |
| Björn Brala (bbrala) |
Ah kk |
| Björn Brala (bbrala) |
Was wondering if I misread, appearantly so.Still, such a module might be helpfull. Could even just test against 9.5 (and 10) and fail if it doesn't work :thinking_face: |
| xjm |
So the manual testing will be roughly like:Install 9.3.x with the core module and create data and configuration such as it allows.Update to 9.5.x with the contrib module version for one that is ready to go (can be a dev version), following the instructions on the removed modules handbook pageUpgrade to 10.0.x and commit the removal patch (or just rm -rf the module dir as a testing step if the removal patch is not applying) Make sure your data, functionality, etc. works as expected |
| xjm |
HAL is a good one to test first since it's simple. I imagine Color and RDF have some trickery around their library dependencies, just like QuickEdit did. Separare thread to check on the status lately. |
| Björn Brala (bbrala) |
1a. Test the functionality and write down the steps.4. Test the functionality with your own testscript. |
| xjm |
Also, devel_generate was very useful back in the day for generating test data |
| xjm |
So we could add that as a suggestion for modules that have or act on data |
| Björn Brala (bbrala) |
Good point |
| Björn Brala (bbrala) |
Although not always very realistic data. But giid enough for these purposes I guess. I do feel that it is sometimes a little hard to understand what is what if you devel generate. But that probably just me :sweat_smile: |
| xjm |
I would very much love it if an enthusiastic, detail-oriented, experienced manual tester did the first example manual upgrade testing issue, so we could give feedback and improve the process with them -- that makes it easier for more people to follow the model in the future |
| Björn Brala (bbrala) |
Hmm |
| xjm |
(webchick is the ultimate person for this but doens't have bandwidth with her new job, I think) |
| xjm |
Anyone with a thorough style and that special gift for breaking things :smile: |
| Björn Brala (bbrala) |
:smile: |
| Björn Brala (bbrala) |
I'm tempted to spread myself even thinner :disappointed: |
| xjm |
It might also be fun/helpful to pair with someone too |
| xjm |
Or seek them as reviewers for ideas to expand your testing |
| xjm |
@quietone and @benjifisher are two other folks who I can think of who are good at this kind of thing also |
| Björn Brala (bbrala) |
Who also do loads of work already :stuck_out_tongue_winking_eye: |
| xjm |
Yep lol |
| Björn Brala (bbrala) |
I'll commit 2 hours this week to try and do hal. I worked on the deprecation of that and properly understand composer. The fact I do jsonapi might help in that specific case. Also I like to do as little manually as possible, so I'll think about some sort of combination of umami and Devel with proper steps. |
| xjm |
@Björn Brala (bbrala) That'd be great!!!! Then we can build on that for the next one and etc. |
| xjm |
@Björn Brala (bbrala) If there isn't one already, I'd file a child issue under the HAL deprecation meta for this |
| Björn Brala (bbrala) |
Yeah, hopefully hehe. I'll be on vacation next week, but that also means less busy evenings hehe |
| xjm |
That you can use to document your findings |
| Björn Brala (bbrala) |
Yes will do that |
| Björn Brala (bbrala) |
And I'll tag you and gabor on slack so you know about the issue. |
| Björn Brala (bbrala) |
Should I not say hi next Tuesday :sweat_smile: |
| benjifisher |
I am flattered. |
| Björn Brala (bbrala) |
Hehehe |
| Björn Brala (bbrala) |
There is still room for some help though, is it ok if I tag you after posting my efforts? |
| Björn Brala (bbrala) |
Even if its just a sanity check of someone more experienced around Drupal. :) |
| benjifisher |
No promises, but it is OK to tag me. |
| Björn Brala (bbrala) |
Best effort is fine ^^ Thank you :slightly_smiling_face: |
| quietone |
I've added this to my list to update the 'testing' step in the policy based on this discussion. |
| catch |
@Björn Brala (bbrala) @xjm HAL and aggregator are already removed from the 10.x branch and have stable 10.x releases, so #3299191: Ensure that modules moved to contrib in 10.0.x are installed as dependencies of other contrib modules shouldn't be blocked at all afaik. We can't test it with not-yet-removed modules, but whatever happens for those two will be the same for the others so implications for the facade are the same. So any 10.x module with a dependency on those two would be useful I think. (edited) |
| xjm |
Right, the façade piece is just one part of it; also making sure that there aren't other unexpected errors when upgrading is another part of it |
| Björn Brala (bbrala) |
I know I tested the path for hal earlier when published a release for the contrib version. I'll just do it again and document the steps into something we can document for the other modules. |
| Björn Brala (bbrala) |
I wish there was a 'dependends' list somewhere in project pages, gitlab or drupals composer package repository. So usefull xD https://packagist.org/packages/swisnl/json-api-client/dependents?order_b... |
| catch |
All the information is in the db so a good feature request against drupalorg I think. |
| Björn Brala (bbrala) |
Star and like :wink: - #3299905: Show "dependants" for modules |
| catch |
Closed as a duplicate of https://www.drupal.org/project/project_dependency/issues/760890 |
| Björn Brala (bbrala) |
Aww I actually search the drupalorg queue. Sorry! |
| catch |
I would have too, found this one via google :wink: |
| Björn Brala (bbrala) |
Haha, that's hmm, perhaps better :sweat_smile: |
| Björn Brala (bbrala) |
Seems actually has a working patch even |
| catch |
@Björn Brala (bbrala) now I'm not sure it's a real duplicate, seems to be showing dependencies for releases but not dependants on project pages (although that's mentioned in the summary) |
| Björn Brala (bbrala) |
Hmm, I'll read through it then. |
| catch |
I think we could change the issue summary, and link to yours - sorry jumped the gun on marking as duplicate. |
| Björn Brala (bbrala) |
Hehe, you mark a lot as duplicate during your days probably, don't worry about it :sweat_smile: But yeah that seems like an good idea. |
| catch |
Switched things around a bit. |
| Björn Brala (bbrala) |
God so many spelling errors. Thank you. |
| Björn Brala (bbrala) |
Started a new thread for the manual testing script: https://drupal.slack.com/archives/C014CT1CN1M/p1659019140392179 |
| xjm |
#3300274: Fix info.yml dependency namespacing is relevant |
| xjm |
@catch ^ re: composer replace section; we didn't regress for modules being moved out of core, but we did for modules that were moved into core per @mglaman |
| xjm |
Although arguably that constraint was just wrong before, but it is a disruption anyway |
| Björn Brala (bbrala) |
Yeah that constraint was wrong. It depends on the contrib. Still though, there might be more. Perhaps I should check the ecosystem :thinking_face: |
| mglaman |
media_acquiadam had issues in a submodule, still has media:media :facepalm: |
| mglaman |
which I still don’t know how or why the CI tooling is failing on a submodule info.yml (edited) |
| catch |
@xjm ah that's similar to #3295096: Drupal no longer 'replaces' composer namespaces for core modules |
| Björn Brala (bbrala) |
The package endpoint for Drupal does do some weird things for modules that moved out of core. Posted here #3299191: Ensure that modules moved to contrib in 10.0.x are installed as dependencies of other contrib modules#comment-14631373But check the versions/source when doidng a composer showvagrant@1WNLK63:/home/localcopy/drupal-module-file-replace $ composer show drupal/hal -a -l
Info from https://repo.packagist.org: #StandWithUkraine
name : drupal/hal
descrip. : Hypermedia Application Language (HAL)
keywords :
versions : 10.0.0-alpha2, 10.0.0-alpha1, 9.4.3, 9.4.2, 9.4.1, 9.4.0, 9.4.0-rc2, 9.4.0-rc1, 9.4.0-beta1, 9.4.0-alpha1, 9.3.x-dev, 9.3.20, 9.3.19, 9.3.18, 9.3.17, 9.3.16, 9.3.15, 9.3.14, 9.3.13, 9.3.12, 9.3.11, 9.3.10, 9.3.9, 9.3.8, 9.3.7, 9.3.6, 9.3.5, 9.3.4, 9.3.3, 9.3.2, 9.3.1, 9.3.0, 9.3.0-rc1, 9.3.0-beta3, 9.3.0-beta2, 9.3.0-beta1, 9.3.0-alpha1, 9.2.x-dev, 9.2.21, 9.2.20, 9.2.19, 9.2.18, 9.2.17, 9.2.16, 9.2.15, 9.2.14, 9.2.13, 9.2.12, 9.2.11, 9.2.10, 9.2.9, 9.2.8, 9.2.7, 9.2.6, 9.2.5, 9.2.4, 9.2.3, 9.2.2, 9.2.1, 9.2.0, 9.2.0-rc1, 9.2.0-beta3, 9.2.0-beta2, 9.2.0-beta1, 9.2.0-alpha1, 9.1.x-dev, 9.1.15, 9.1.14, 9.1.13, 9.1.12, 9.1.11, 9.1.10, 9.1.9, 9.1.8, 9.1.7, 9.1.6, 9.1.5, 9.1.4, 9.1.3, 9.1.2, 9.1.1, 9.1.0, 9.1.0-rc3, 9.1.0-rc2, 9.1.0-rc1, 9.1.0-beta1, 9.1.0-alpha1, 9.0.x-dev, 9.0.14, 9.0.13, 9.0.12, 9.0.11, 9.0.10, 9.0.9, 9.0.8, 9.0.7, 9.0.6, 9.0.5, 9.0.4, 9.0.3, 9.0.2, 9.0.1, 9.0.0, 9.0.0-rc1, 9.0.0-beta3, 9.0.0-beta2, 9.0.0-beta1, 9.0.0-alpha2, 9.0.0-alpha1, 2.x-dev, 2.0.1, 2.0.0, 1.x-dev, 1.0.x-dev, 1.0.1, 1.0.0, dev-1.0.x, dev-1.x, dev-2.x
latest : dev-2.x
type : drupal-module
license : GNU General Public License v2.0 or later (GPL-2.0-or-later) (OSI approved) https://spdx.org/licenses/GPL-2.0-or-later.html#licenseText
homepage : https://www.drupal.org/project/hal
source : [git] https://git.drupalcode.org/project/hal.git 7bbe0f1a38b15b6d39c01848a497c24e6c8ce629
dist : []
names : drupal/hal
support
source : https://git.drupalcode.org/project/hal
requires
drupal/core ^10
requires (dev)
drupal/entity_reference_revisions 1.x-dev
drupal/aggregator 2.x-dev |
| Björn Brala (bbrala) |
My gut almost feel like the contrib should just release as 11.x (or 10.x?) so versions keep making sense :x (edited) |
| Spokje |
"Interesting find"/nice catch @Björn Brala (bbrala)!So it looks like (at least to me) that by using drupal/[name-of-core-module] in the replace: section, Packagist assumes that they are actual composer packages, although they in fact have no composer.json of their own.Maybe we (probably a release manager/DA-member) should send an email to `contact@packagist.org` on how to get rid of those references?https://github.com/composer/packagist/issues/875#issuecomment-388778029 |
| Björn Brala (bbrala) |
It's not packagist but the drupal one I think. And replace section is very common in large frameworks. For example check laravel: https://github.com/laravel/framework/blob/9.x/composer.json |
| Björn Brala (bbrala) |
And symfony "symfony/composer.json at 6.2 · symfony/symfony · GitHub" https://github.com/symfony/symfony/blob/6.2/composer.json |
| catch |
Yeah it'll be project_composer. We were able to remove the replace because it uses a database query instead now, but it'll be that database query that's providing these. We possibly might want to keep it like this preferring core until the removed modules are all stable again though. |
| Björn Brala (bbrala) |
HAL has been removed and everything. I think it should not be provided by core in 10 |
| Björn Brala (bbrala) |
Which is why i contemplated a major of 10 for hal as first release, since that is pretty much the next version. if you look at it from a semver standpoint. |
| Björn Brala (bbrala) |
hal has a 9.4 and a 10.0 release though (1.x and 2.x) so not sure how i would do that then |
| Spokje |
Was just about to make a comment like that ^The 9.x versions would clash (I think?) |
| catch |
Could it do 10 and 11 for hal? |
| Spokje |
My tiny mind would be confused to use 10 on D9 and 11 on D10 TBH, although I think technically it would/could/should work |
| Björn Brala (bbrala) |
That would complicate the versions even more |
| catch |
Could do 9.10 and 10.0 a bit weird but would be higher than 9.5 |
| Björn Brala (bbrala) |
ah you are right catch |
| Björn Brala (bbrala) |
tbh i think to keep versions sane that would be the best way |
| Björn Brala (bbrala) |
But we do need to remove removed modules from the 10.0.x branch although a astable release from contrib will overide the dev release by drupal. BUT we would supply 2 dev releases in the 10 range, since drupal provides it AND the contrib module |
| Björn Brala (bbrala) |
Ping @larowlan since you are also (and more active) a maintainer for HAL, what do you think? (edited) |
| Spokje |
I like:The pragmatic approachMatching major core version/contrib version numbersThe sheer optimism we won't get up to Drupal core 9.10.xI do think this needs to be documented somewhere and current and future maintainers of deleted modules should be aware. (edited) |
| Björn Brala (bbrala) |
A long time it was common for laravel packages to follow laravel major versions. |
| Björn Brala (bbrala) |
Although not su much nowadays :slightly_smiling_face: |
| Björn Brala (bbrala) |
So we agree a possible solution to this predicament:Release contrib versions of removed packages with the same major as core (9.10.x, 10.0.x)Make an issue on project_composer to remove the drupal provided 10.0.x for removed packages.Make an issue to adjust the whole removal process to facilitate these steps.Profit. |
| Björn Brala (bbrala) |
Still want to wait for @larowlan though, since I would mix up the pretty versions/releases currently released :) |
| catch |
We could get project_composer to remove the core versions maybe, but wouldn't want that to happen on 9.3 |
| Björn Brala (bbrala) |
Tbh only on 10 |
| Björn Brala (bbrala) |
Not sure we want to remove from 9.4 already, we are only removing in 10.x |
| Björn Brala (bbrala) |
this brings another predicament though, 9.5 is going to be released. |
| Björn Brala (bbrala) |
With HAL still in core. |
| Spokje |
I think since this an issue on all removed core modules, we need more people to sign of on the proposed solution?Maybe a topic for the next d10-ready meeting?Or am I too much in Doom-'n-Gloom land? |
| Björn Brala (bbrala) |
Probably, although catch and xjm are both in this thread :wink: |
| catch |
If we remove it from packagist, it'll still only install contrib hal if you install hal or a dependent module via composer so I think that would still be fine on 9.4/9.5 but it's not straightforward. This was a lot easier without composer :wink: |
| Björn Brala (bbrala) |
why isn't it straightforward? |
| Björn Brala (bbrala) |
have the right version point at the right repo and dont supply conflicing versions. |
| Björn Brala (bbrala) |
which would probably mean it IS as easy as just composer require |
| Björn Brala (bbrala) |
hmm |
| catch |
I mean it's not straightforward whether we definitely want the contrib version to install on 9.4 sites compared to now where it only does so if you request a specific version, but overall, I think that's what we want. |
| Björn Brala (bbrala) |
Yeah, that way the migration could be quite easy. Although the current stuff also works. I didnt actually try and see hwat composer outdated would print... That would be interesting. |
| Björn Brala (bbrala) |
ill do some testing tonight see if there is more to it. |
| Björn Brala (bbrala) |
Well it does seem composer doesnt get confused, what i assume is that it checks the actual repository (contrib) what versions it can install |
| Björn Brala (bbrala) |
pvagrant@bjorn-pc:/home/localcopy/drupal-module-hal$ composer outdated
Color legend:
- patch or minor release available - update recommended
- major release available - update possible
asm89/stack-cors 1.3.0 v2.1.1 Cross-origin resource sharing library and stack middleware
composer/composer 2.2.17 2.3.10 Composer helps you declare, manage and install dependencies of PHP projects. It ensures you have the right stack everywhere.
composer/installers v1.12.0 v2.1.1 A multi-framework Composer library installer
composer/pcre 1.0.1 3.0.0 PCRE wrapping library that offers type-safe preg_* replacements.
consolidation/log 2.1.1 3.0.0 Improved Psr-3 / Psr\Log logger based on Symfony Console components.
consolidation/site-process 4.2.0 5.0.0 A thin wrapper around the Symfony Process Component that allows applications to use the Site Alias library to specify the target for a remote call.
doctrine/reflection 1.2.3 1.2.3 The Doctrine Reflection project is a simple library used by the various Doctrine projects which adds some additional functionality on top of the reflection functionality that comes with PHP. It allows you to get the reflection information about classes, methods ...
Package doctrine/reflection is abandoned, you should avoid using it. Use roave/better-reflection instead.
drupal/hal 1.0.1 2.0.1 Serializes entities using Hypertext Application Language.So i'm guessing the whole available versions thing is not really a problem. |
| larowlan |
Is the TL;dr that we need a 9.4 and 10.0 branch of Hal module? |
| Spokje |
Is the TL;dr that we need a 9.4 and 10.0 branch of Hal module?I think that was always the case.I think (I should really stop doing that, makes my head hurt) the issue here is if the 9.x branch should be 9.10.x and the 10.x branch should be 10.x.I lost the plot a bit on where we stand on this at the moment. |
| Björn Brala (bbrala) |
My last conclusion is that it doesn't seem to matter.... |
| Björn Brala (bbrala) |
Even though composer it giving some weird version lists |
| Björn Brala (bbrala) |
When testing it seems it doesn't resolve to the higher versions provided by core. |
| Björn Brala (bbrala) |
So I assume it's a false alarm |
| Spokje |
Yay to false alarms!:thinking_face: |
| Björn Brala (bbrala) |
It confuses the hell out of me. :sweat_smile: |
| Gábor Hojtsy (he/him) |
Raised by @andypost see #3283358: [META] Make Drupal 9/10 compatible with PHP 8.2 |
| andypost |
Last week I been able to minimize amput of failed tests to 116, few blockers are commited but there's list of main ones |
| andypost |
[#3291553]needs to require composer >=2.3.6and 2 more dependencies one of which in Alpha release |
| andypost |
[#3275851]few issues commited but remaining ones are tricky as needs split to fix mocks and add attributes, also not clear fate of phpstan-L0 fixes (big hunk of the patch) |
| andypost |
and the primary blocker is serialization, we can't add _serviceId to every service so core need to replace current dependecy serialization #2531564: Fix leaky and brittle container serialization solution |
| andypost |
I did split #3299828: Stop storing Settings singleton in object properties which may need better title but basically require to stop serialize settings service |
| andypost |
#3299327: Replace deprecated static::method() calls for PHP 8.2 is ready to be commitedand follow-up issue updated which is not blocker but syntax polishing |
| longwave |
@andypost the settings one doesn't look too hard if we can just store the settings value in each constructor instead of the object |
| andypost |
@longwave but it will need to change constructors ~10 services |
| longwave |
we can still inject Settings object? just not store it inside the service? |
| longwave |
so that is not so bad, no deprecation needed |
| andypost |
ah, yes |
| longwave |
(unless the service is extended in contrib i guess, and relies on $this->settings) (edited) |
| andypost |
I hope @chx (he/him) will comment more details, it should fix a bunch of tests |
| andypost |
also remaining failures with plugin discovery and D7 migrations not clear how to fix #3295821: Ignore: patch testing issue for PHP 8.2 attributes#comment-14619130probably it's a result of incimplete serialization patch |
| chx (he/him) |
the issue at #2531564: Fix leaky and brittle container serialization solution |
| chx (he/him) |
currently we have service objects carrying their own service id and we want to replace that |
| chx (he/him) |
how do we that? well, the issue suggests creating a mapping of "Instance #0 of ChainedFastBackend has a service id cache.data" and so forth |
| chx (he/him) |
however, this is not a stable mapping because objects can come and go |
| chx (he/him) |
it's not just Settings although certainly it's the main culprit |
| andypost |
approot service remains also needs workaround |
| longwave |
app root is a container parameter now? |
| andypost |
in 9.5 it is service #3299664: Ignore: patch testing issue#comment-14624942 |
| chx (he/him) |
just cast it to object |
| chx (he/him) |
well, no because each instance of that will be a different object (edited) |
| chx (he/him) |
shoot |
| chx (he/him) |
I will solve that in an hour when I am more properly here |
| xjm |
BTW I unpostponed #3295520: Update dependencies for Drupal 10.0 and 9.5 now @longwave @andypost so that can be updated with whatever other dep updates we need. |
| andypost |
@xjm are composer and vfs patches could be merged into this one? |
| xjm |
It is worth doing them separately when we increase a constraint or add a new dependency, because those sorts of individual changes require specific release notes, sign off, etc. Just wanted to let you know that composer changes are no longer on hold until the next release basically :smile: (edited) |
| andypost |
Finally https://github.com/bovigo/vfsStream/releases/tag/v1.6.11 is out, updated isssues and patches! (edited) |
| catch |
Ooooh that is good, wasn't expecting it that quick. |
| andypost |
@catch please re-queue https://www.drupal.org/pift-ci-job/2435565 |
| xjm |
@andypost The DA is troubleshooting and has asked us not to queue a ton of jobs. DrupalCI is having issues. |
| andypost |
@xjm that's ping @mixologic to give sane logs |
| mixologic |
Yeah, we may have it narrowed down to an attempt to move the project analysis job to gitlabci. Still investigating. The real issue here is that we probably do not have a 'right sized ' environment to move all testing to gitlabci without doing more infra work. we'll see |
| Gábor Hojtsy (he/him) |
Oh that job attempting to move to gitlab CI broke stuff on DrupalCI? |
| mixologic |
Well, not broke stuff. Remember when we ddos'ed ourselves from jenkins trying to parallelize too much? |
| mixologic |
well, we're sorta going through the same types of growing pains here |
| Gábor Hojtsy (he/him) |
Hah |
| mixologic |
but instead of it being the job cloning too much, the job is processing a lot of artifacts, which now that they live in gitlab, puts strain on our gitlab server. |
| mixologic |
which, in turn, caused it to take entirely too long to just 'clone core' |
| mixologic |
which is why the core jobs were failing. |
| mixologic |
Someday I wish we could prune cores' git history and start fresher without commits going all the way back 15 years or however long it is. |
| mixologic |
maybe post drupal 7 support or something |
| Gábor Hojtsy (he/him) |
Such good history though! |
| Gábor Hojtsy (he/him) |
@Björn Brala (bbrala) can stop all the gitlabci contrib compatibility jobs :slightly_smiling_face: |
| Björn Brala (bbrala) |
@mixologic I broke gitlab? |
| Björn Brala (bbrala) |
oops |
| mixologic |
Well, maybe |
| mixologic |
likely |
| xjm |
I was also blamed earlier; let's start a club :smile: |
| Björn Brala (bbrala) |
It runs 10 jobs in parallel, running 2 threads that do a lot of IO (checkouts and such) |
| Björn Brala (bbrala) |
It does that for a while, the whole run is 53 jobs. |
| mixologic |
and not 'broke it' but I think its more the artifact processing.. how many artifacts does it create? |
| Björn Brala (bbrala) |
eh |
| Björn Brala (bbrala) |
50k perhaps |
| Björn Brala (bbrala) |
:s |
| Björn Brala (bbrala) |
i can check, it commits it to a branch |
| mixologic |
yeah, our 'geo' server is probably trying to synchronize those over as a backup as well |
| Björn Brala (bbrala) |
hmm |
| Björn Brala (bbrala) |
the artifacts are zips though |
| Björn Brala (bbrala) |
containeing a lot of files. |
| Björn Brala (bbrala) |
brb |
| mixologic |
okay, so thats probably not as bad |
| Björn Brala (bbrala) |
its one artifact per run, temporary then combines all the artifacts to a single artifact in the last job |
| Björn Brala (bbrala) |
it then commits those files to a orphan branch so we kinda have a history (and eventually a place to link to for errors and such) |
| Björn Brala (bbrala) |
but i can imagine 9 jobs of this on a single worker, while trying to checkout full core (why isnt it a shallow copy btw?) might be overdoing it regarding IO |
| Gábor Hojtsy (he/him) |
The commits would result in lots of syncing though? |
| Björn Brala (bbrala) |
hmm |
| Björn Brala (bbrala) |
i guess, but not sure how it is synced and the amount of load that puts on stuff. |
| Björn Brala (bbrala) |
The branch is 52k files that get committed. Also the job does a find -exec sed to normalize the results. (edited) |
| Björn Brala (bbrala) |
@mixologic if you want i can at least make it do less work at the same time. Make it single thread and perhaps limit to 5 paralel jobs? |
| xjm |
Maybe all this should go in a different thread? |
| Björn Brala (bbrala) |
oops |
| Björn Brala (bbrala) |
yes |
| xjm |
@andypost Queued https://www.drupal.org/pift-ci-job/2436847 |
| Björn Brala (bbrala) |
https://drupal.slack.com/archives/C014CT1CN1M/p1658861790429699 |
| andypost |
Build Successful so next blocker is $_serviceId (edited) |
| chx (he/him) |
I had a good long discussion with @neclimdul and we think the patch in the issue might be viable if we add a switch to container set so that calling it makes sleeping impossible |
| chx (he/him) |
don't dynamically swap services while rebuilding the container is reasonable |
| chx (he/him) |
should be overridable in settings |
| chx (he/him) |
if you have that switch in place reasoning becomes easy because the container itself keeps the objects alive |
| Gábor Hojtsy (he/him) |
Raised by @danflanagan8 #3083275: [meta] Update tests that rely on Classy to not rely on it anymore |
| danflanagan8 |
The spreadsheet tracking module-by-module progress is here: https://docs.google.com/spreadsheets/d/1OAAnJ7Q3CcM9oeGyXkMrgw-6QjAYEkkn... |
| xjm |
As mentioned, this is one of our "really really really should-have" things not in the must-have list; we'd really like to be able to stabilize Starterkit and deprecate Classy. So any reviews to help with this are greatly appreciated. |
| danflanagan8 |
We’re down to 8 modules that still use Classy in tests, plus some non-module tests in Tests/Drupal etc |
| danflanagan8 |
Most of the remaining modules are big ones like views, system, and ckeditor5 |
| Gábor Hojtsy (he/him) |
Oh even ck5?! |
| danflanagan8 |
If we can’t get everything converted to Stark in time, would we ever consider updating remaining classy tests to use starterkit and then make a followup to convert from starterkit to stark? |
| xjm |
@danflanagan8 Yep that's always been the fallback plan, if Starterkit gets stable first otherwise and this is still in progress |
| danflanagan8 |
That’s good to hear |
| danflanagan8 |
I know that I have some contrib modules that use classy in tests. and I’m guessing I’m no the only one. Is that something that the Rector Bot would potentially be able to help with with when classy gets removed from core? cc: @mglaman |
| Gábor Hojtsy (he/him) |
Are the changes required automatable? |
| xjm |
@Gábor Hojtsy (he/him) Not really in my experience |
| xjm |
There are lots of different patterns of usage, and different assumptions based on the specific template under test |
| xjm |
I mean we can easily str_replace the required theme and see what fails, but beyond that.... |
| mglaman |
Rector only reads PHP. Doesn't really fix library usage. I did catch Checklist API uses classy/progress vs core/drupal.progress |
| danflanagan8 |
Yes, I hadn’t thought about library usage. I guess I was hoping there could be some magic that would notice $defaultTheme = 'classy' and then add @requires theme classy or something like that. :magic_wand: |
| mglaman |
oh you mean in tests? |
| mglaman |
well Drupal's @requires is broken anyways |
| danflanagan8 |
uh oh! |
| danflanagan8 |
Yeah, I just meant in tests. |
| mglaman |
see https://www.drupal.org/i/3261817 https://git.drupalcode.org/project/acquia_dam/-/blob/1.0.x/tests/src/Ker... |
| danflanagan8 |
Maybe a simpler thing to do would be for the Bot to change $defaultTheme = 'classy' to $defaultTheme = 'starterkit' in contrib tests? |
| mglaman |
I think so. We could make a drupal-rector rule for that |
| mglaman |
If not set, set to stark , if set and is classy change to starterkit |
| danflanagan8 |
That might end up being really useful. I see 18 uses of $defaultTheme = 'classy' in the contrib modules on a client project I’m currently on. (Spread over 12 or so of the ~50 contrib modules in the project) (edited) |
| mglaman |
opening an issue for drupal-rector |
| danflanagan8 |
Thanks! Here’s the link if anyone else is interested: #3299835: Provide $defaultTheme for functional tests |
| Kristen Pol (she/her) |
Question... what's the ETA when we shouldn't be creating subthemes anymore? There was a discussion in the event organizers' Event Platform meeting where we were trying to figure out if we do a subtheme or use starterkit (edited) |
| xjm |
There's some nuance missing there which is what the base theme is |
| Kristen Pol (she/her) |
Olivero at the moment |
| mglaman |
you can almost starterkit olivero |
| xjm |
So Starterkit only REALLY pertains to subtheming Classy or not |
| Kristen Pol (she/her) |
ah, I didn't realize that |
| Kristen Pol (she/her) |
I thought it was a generic tool |
| xjm |
There are two parts: the generator and the base theme |
| xjm |
The generator can be used for anything |
| xjm |
The base theme is essentially "Classy but without perma-BC" |
| xjm |
One sec, there's a helpful diagram in one of the blog posts, digging it out |
| Kristen Pol (she/her) |
cool, thanks! |
| xjm |
@Kristen Pol (she/her) https://www.drupal.org/files/image1_15.png from https://www.drupal.org/about/core/blog/new-starterkit-will-change-how-yo... |
| xjm |
So if you want to subtheme Olivero you can do that, although that's never been officially supported by core. All core themes besides Classy and Stable are internal and may break BC in minors. (edited) |
| xjm |
If you want to use Starterkit to generate a theme from Olivero, you could theoretically do that too, but by default it's meant to replace the role Classy played |
| Kristen Pol (she/her) |
interesting, thank you for the clarification, very helpful :thumbsup: |
| mglaman |
to extend on what xjm said, correct starterkit is a replacement to classy. but the generator can support contrib themes identifying as a starterkit |
| xjm |
So it kinda depends whether you can take time to update your theme each minor anytime Olivero breaks BC (advantage: get more bug fixes and new stuff for new core UIs), or fork from Olivero as of 9.4.0 (but have to add its further improvements manually using diffs between different versions) |
| mglaman |
which technically in the future Olivero could be one supporting the “fork and forget” |
| Kristen Pol (she/her) |
very cool... I will leave it to the frontend Event Platform folks to decide how to use this information :laughing: because I don't do a huge amount of frontend these days... but the upshot is we want to have an Event Platform theme using the Event Platform config that is pretty much good enough for many simple events that don't want to do much of their own theming... hoping it will look something like https://www.fldrupal.camp/ but with Florida image/logo swapped and orange color switched out... that is Olivero-based as far as I understand |
| xjm |
Yep, so the equivalent of "University base theme" in the diagram. Whether to subtheme from Olivero or fork from Olivero depends on the kind of maintenance. If it's kept very close to core Olivero, subtheming is probably the easier/safer route. |
| xjm |
But if there's a lot of custom CSS and templates, forking and making their own starterkit base theme might be a better idea |
| Kristen Pol (she/her) |
Gotcha :+1::+1::+1::+1::+1: Thanks a ton :heart: |
Comments
Comment #2
gábor hojtsyComment #3
spokje:two: Deprecating/Removing core modules.
:two:.:one: Do we have a must-have list of modules we must delete before the 10.0.0 release?
Currently our efforts are wide-spread. Looking at the fast approaching deadline for 10.0.0 and the current (non-)speed of progress on deprecation/removal issues, I think we must focus on just a few modules that we can actually achieve removal of.
:two:.:two: Currently no theme is deprecated yet. Are we planning to deprecate themes (bartik/seven/stable/stable9) in 10.0.0? If so, do they (in general) have to follow the same procedure as modules?
:two:.:three: We decide we need to manually test any core module before we commit the actual deletion issue. For core modules we already deleted we also have to manually do testing.
Currently we haven't decided how to test, so we really can't guide people on how to do this.
Since the range of modules and their functionality can be huge, I think we can/should keep the testing instructions rather "generalized", so we can add them to the procedure-documentation and also create issues in the d.o. queue for each module so people can start working on those.
Since the working of the module in (now) 9.x is tested by core tests and the working of the contrib incarnation of the core module is tested by the core-tests-moved-with-the-module in (now) 10.0.x, I think we only need to test the main thing that we can't test with TestBot: Having the core module enabled, updating to (now) 10.0.x and composer-requiring the contrib incarnation.
Would something general like instructions on the above procedure and testing that the module's functionality keeps the same and no module content (think for example feeds in case of the aggregator module) is lost, be enough or do we need to be more specific?
:two:.:four: We agreed that if no manual testing has taken place or problems that have emerged during manual testing aren't fixed, we should roll-back the removal (if it has already taken place) or postpone it on fixes.
Rolling back might not be as "easy" as reverting a commit. With (for example) aggregator we will re-gain the dependency on laminas/laminas-feed and laminas/laminas-diactoros
Looking at the above, should we have an "exit-strategy" on what to do when we reach a certain deadline-date?
If so would something like 2 weeks before we 10.0.0 will be released be a good deadline?
Comment #17
gábor hojtsySaving notes.
Comment #18
gábor hojtsy