Skip to content

[FrameworkBundler] Fix cache:clear with buildDir #39360

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 15, 2020
Merged

[FrameworkBundler] Fix cache:clear with buildDir #39360

merged 1 commit into from
Dec 15, 2020

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Dec 7, 2020

Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39232
License MIT
Doc PR

Since #36515 there are 2 caches dir cacheDir and buildDir. For BC reason both points to the same folders, but when app don't use the same folder, many thing are broken:
This PR fixes several issues introduces by the above PR:

  • some files are persisted in the wrong folder (App_KernelDevDebugContainerDeprecations.log, App_KernelDevDebugContainer.xml)
  • LoggerDataCollector looks into cache_dir, while Compiler.log is written in build_dir and Deprecations.log were written in cache_dir before this PR
  • the logic that mirror cacheDir into buildDir at the end of CacheClearCommand does not make sens when cache_dir and build_dir are not identical.
  • Files generated in cacheDir are trashed at the end of CacheWarming (initial issue)

@carsonbot carsonbot added this to the 5.2 milestone Dec 7, 2020
@jderusse jderusse force-pushed the fix-warmup branch 5 times, most recently from 5b7dcc1 to e8e3aab Compare December 8, 2020 16:56
@jderusse
Copy link
Member Author

jderusse commented Dec 9, 2020

FYI test in red because test in FWB requires fix Kernel form this same PR (that's why in changed composer.json to 5.2.1)

@nicolas-grekas
Copy link
Member

(the tests on Windows might need some love.)

@jderusse
Copy link
Member Author

(the tests on Windows might need some love.)

Fixed with my must favorite trick. Doing nothing... ^^

In fact, previous failure were very strange: I guess appveyor had FS issue
Failed to remove non-empty directory: Fixtures/var
Permission denied: Fixtures/var/cache
Failed to create dir: var/cache/resetting

@Sylry Sylry mentioned this pull request Dec 12, 2020
Closed
@nicolas-grekas
Copy link
Member

(rebase needed)

jderusse
jderusse commented Dec 14, 2020
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Tests/Command/CacheClearCommand/CacheClearCommandTest.php

class NoBuildDirKernel extends TestAppKernel
Copy link
Member Author

Choose a reason for hiding this comment

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

Could you have a look @chalasr ? Previous code can't work, because kernel.build_dir is now needed to build the container (and now needed to initialize the cache:clear command).
I refactored the code t build a container and then remove the parameter from the dumped container.

@alongosz alongosz mentioned this pull request Dec 14, 2020
Merged
2 tasks
@alongosz
Copy link

Hey Guys, just wanted to let you know that this PR resolves our blocker in Ibexa DXP preventing us from upgrading to Symfony 5.2. Thanks 👍

@chalasr chalasr self-requested a review December 15, 2020 07:10
@chalasr
Copy link
Member

chalasr commented Dec 15, 2020

Thanks for fixing this bug @jderusse.

@chalasr chalasr merged commit a7fd98b into symfony:5.2 Dec 15, 2020
@chalasr chalasr mentioned this pull request Dec 15, 2020
Closed
This was referenced Dec 15, 2020
Closed
Closed
@Kocal
Copy link
Member

Kocal commented Dec 15, 2020

Thanks @jderusse ❤️ ❤️

@alongosz
Copy link

Thanks @jderusse 🎉
When can we expect a patch release / tag? :-)

@derrabus
Copy link
Member

By the end of the month, usually.

@lserwatka
Copy link

@derrabus we are launching new Ibexa DXP 3.3.0 on December 30th. We are eager to use Symfony 5.2 as all our tests are passing with this fix.

Could you make a Symfony 5.2.1 release before December 30th?

@derrabus
Copy link
Member

Could you make a Symfony 5.2.1 release before December 30th?

I'm pretty confident that 5.2.1 happens before Dec 30th. 😃 But please test against 5.2-dev in the meantime to make sure the changes work for you.

@jderusse jderusse deleted the fix-warmup branch December 15, 2020 11:49
@mnapoli
Copy link
Contributor

mnapoli commented Dec 15, 2020

Thank you @jderusse!

@Wirone
Copy link
Contributor

Wirone commented Dec 16, 2020

First of all, thanks for the fix - I've encountered this problems in the project created with CLI binary few days ago and had to downgrade to 5.1 to continue working on it.

Personally I think that fixes like this one should be considered as hotfixes and released as soon as possible. Symfony 5.2.0 was released 16 days ago and is really unstable because of problems with cache (at least in dev environment for development purposes). And now we hear that new version will be released at the end of the month which means for ~30 days straight really broken version will be installed by default by symfony new ... and possibly upgraded to in already existing apps. That's... quite interesting 🤔

@jderusse
Copy link
Member Author

keep calm and run rm -rf var/cache/*

@alongosz alongosz mentioned this pull request Dec 18, 2020
Closed
@fabpot fabpot mentioned this pull request Dec 18, 2020
Merged
@mhujer mhujer mentioned this pull request Dec 21, 2020
Closed
@Wirone
Copy link
Contributor

Wirone commented Dec 23, 2020

Thank you @fabpot for earlier release 👍

@vudaltsov vudaltsov mentioned this pull request Aug 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
10 participants
@jderusse @nicolas-grekas @alongosz @chalasr @Kocal @derrabus @lserwatka @mnapoli @Wirone @carsonbot