Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpKernel] Add$kernel->getBuildDir() to separate it from the cache directory#36515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfrommnapoli:tmp-dir
Aug 21, 2020

Conversation

@mnapoli
Copy link
Contributor

@mnapolimnapoli commentedApr 21, 2020•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#23354
LicenseMIT
Doc PRsymfony/symfony-docs#...

In order to support deploying on read-only filesystems (e.g. AWS Lambda in my case), I have started implementing#23354.

This introduces$kernel->getBuildDir():

  • $kernel->getBuildDir(): for cache that can be warmed and deployed as read-only (compiled container, annotations, etc.)
  • $kernel->getCacheDir(): for cache that can be written at runtime (e.g. cache pools, session, profiler, etc.)

I have probably missed some places or some behavior of Symfony that I don't know. Don't consider this PR perfect, but rather I want to help move things forward :)

TODO:

  • Changelog
  • Upgrade guide
  • Documentation

magnetik, Nemo64, sstok, Kocal, linaori, fritzmg, ogizanagi, shouze, gmponos, monteiro, and 5 more reacted with thumbs up emojipizzaminded, tyx, monteiro, faizanakram99, maximepiton, and johnny1991 reacted with heart emoji
publicfunctiongetTmpDir()
{
// Returns $this->getCacheDir() for backward compatibility
return$this->getCacheDir();
Copy link
ContributorAuthor

@mnapolimnapoliApr 21, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Here is the core of the PR :)

For backward compatibility, I return the current cache directory.

In future versions, this could be for examplevar/build. In the meantime, users can override this method to set it to a different path, for example/tmp.

alexander-schranz reacted with thumbs up emoji
@mnapolimnapoliforce-pushed thetmp-dir branch 2 times, most recently from61bd29e tofee57fbCompareApril 21, 2020 12:40
@nicolas-grekasnicolas-grekas changed the title#23354 Add$kernel->getTmpDir() to separate it from the cache directory[HttpKernel] Add$kernel->getTmpDir() to separate it from the cache directoryApr 21, 2020
@nicolas-grekasnicolas-grekas added this to thenext milestoneApr 21, 2020
@javiereguiluz
Copy link
Member

This solution probably makes a lot of sense ... but at first seems confusing:

Problem:

  • cache/ is read+write, my server is read only

Solution:

  • let's makecache/ read-only
  • let's create a newtmp/ dir which is read+write
linaori reacted with thumbs up emoji

@mnapoli
Copy link
ContributorAuthor

@javiereguiluz yes, this was brought up in#23354 (comment) and I think that's a valid alternative: keep cache as-is, introduce a newbuild directory.

I'm waiting for feedback on this new approach, I started looking into it a pull request as well but haven't opened it yet.

@fabpot
Copy link
Member

This is a good move IMHO.

Reading the related issue, I think we should get the naming right. Keeping thecache/ directory as is and creating a new one for the "static"/"build" cache seems better to me. Let's name itbuild/ for now (I don't have any other name in mind).

jvasseur, rvanlaak, and c33s reacted with thumbs up emoji

@mnapolimnapoli changed the title[HttpKernel] Add$kernel->getTmpDir() to separate it from the cache directory[HttpKernel] Add$kernel->getBuildDir() to separate it from the cache directoryAug 19, 2020
@mnapoli
Copy link
ContributorAuthor

Great! I have updated the PR.

Note that after struggling for a while I did not succeed running the tests locally (Error : Call to undefined method Symfony\Bundle\FrameworkBundle\Tests\Console\ApplicationTest::assertMatchesRegularExpression()), so I'm kind of in the blind here. I've pushed what I have done, we'll see on Travis.

Review is welcome!

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

LGTM (minor comment)

WDYT @symfony/mergers?

@stof
Copy link
Member

@mnapoli are you using the./phpunit script to run tests ?

@weaverryan
Copy link
Member

This makes a lot of sense to me. And the fact that the newgetBuildDir() returnsgetCacheDir() means that we're adding this flexibility, but at zero "learning about some new change" cost to existing users that won't care about this.

Very nice!

}

$io->comment(sprintf('Clearing the cache for the <info>%s</info> environment with debug <info>%s</info>',$kernel->getEnvironment(),var_export($kernel->isDebug(),true)));
$this->cacheClearer->clear($realBuildDir);
Copy link
Contributor

@ogizanagiogizanagiAug 20, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't this command avoid removing the build dir by default? What about adding a--build option?
For the ones not overridingKernelInterface::getBuildDir(), this option won't do anything more.
But for the ones overriding, you might want to call this command to clear the read-write cache, but not the read-only build dir, right?

Copy link
Member

Choose a reason for hiding this comment

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

As a first step, we must clear everything like today.

@mnapoli
Copy link
ContributorAuthor

@stof thanks for the tip, I didn't. Still, when I use it the tests crash with:

PHP Fatal error: Uncaught Error: Call to a member function stop() on null in [...]/src/Symfony/Contracts/HttpClient/Test/TestHttpServer.php:27

Anyway, I don't really want to pollute the discussion with this.

@fabpot
Copy link
Member

Thank you@mnapoli.

@nicolas-grekasnicolas-grekas modified the milestones:next,5.2Oct 5, 2020
@fabpotfabpot mentioned this pull requestOct 5, 2020
chalasr added a commit that referenced this pull requestDec 15, 2020
This PR was merged into the 5.2 branch.Discussion----------[FrameworkBundler] Fix cache:clear with buildDir| 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)Commits-------ea68966 Fix cache:clear with buildDir
javiereguiluz added a commit to EasyCorp/EasyAdminBundle that referenced this pull requestNov 27, 2024
This PR was squashed before being merged into the 4.x branch.Discussion----------Use buildDir for cacheFeature available since Symfony 5.2 -symfony/symfony#36515Commits-------9daaade Use buildDir for cache
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+4 more reviewers

@sstoksstoksstok left review comments

@ogizanagiogizanagiogizanagi left review comments

@fancywebfancywebfancyweb left review comments

@ChemaclassChemaclassChemaclass left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

Add var/tmp / $kernel->getTmpDir()

11 participants

@mnapoli@javiereguiluz@fabpot@stof@weaverryan@sstok@ogizanagi@fancyweb@Chemaclass@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp