Page MenuHomePhabricator

Point amp cache to $WORKSPACE
Closed, ResolvedPublic1 Story Points

Description

Currently, amp caches values in $HOME/.amp, which breaks concurrency. If we cache in $WORKSPACE, jobs can run in parallel. (Verify that composer .cache locks correctly.)

There's already a setting that might do the job, parameters: app_dir, it might be as simple as figuring out how to set that.

Externally tracked on the project page: https://github.com/totten/amp/issues/7

Event Timeline

awight created this task.Mar 7 2015, 11:25 PM
awight raised the priority of this task from to Low.
awight updated the task description. (Show Details)
awight added subscribers: atgo, awight, Aklapper, Ejegg.
awight updated the task description. (Show Details)May 13 2015, 8:33 PM
awight set Security to None.
hashar added a subscriber: hashar.May 13 2015, 8:35 PM

Ah amp is a Symfony 2 application. It comes with a default directory structure that has a cache dir http://symfony.com/doc/current/cookbook/configuration/override_dir_structure.html

Looking at amp source code in src/Amp/Application.php:

class Application extends \Symfony\Component\Console\Application {
...
public static function main($binDir) {
     ...
     $appDir = getenv('HOME') . DIRECTORY_SEPARATOR . '.amp';

So the amp app ends up in $HOME. Maybe it can be overridden by overriding the env var but that might have side effects.

awight added a comment.Jul 2 2015, 5:49 AM

totten closed the upstream feature request! He says the new revision of amp isn't merged into civicrm-buildkit's composer.lock yet, so watch this space.

hashar added a comment.Jul 2 2015, 8:40 AM

Great! Thanks for keeping track of it @awight

awight edited a custom field.Sep 15 2015, 5:49 PM

This is the PR to allow $AMPHOME variable to override the config files path

https://github.com/totten/amp/pull/29

I just did a PR to buildkit https://github.com/civicrm/civicrm-buildkit/pull/192

hashar moved this task from Backlog to Patch proposed upstream on the Upstream board.

Change 256377 had a related patch set uploaded (by Eileen):
Add option to define a separate path for the amp app

https://gerrit.wikimedia.org/r/256377

@awight per your comment A good and quick project for this month: https://phabricator.wikimedia.org/T91911
1:55 until then, we're stuck with a single CRM CI job

I had a crack at a merge on the amp repo but it got a bit stuck pushing up with git review so I've cherry-picked the commit above - https://gerrit.wikimedia.org/r/#/c/256377/ to wmf amp

Actually I'm kinda happy about cherry-pick rather than merge as I didn't enjoy the merge conflict fix much & was only about 90% confident in it. (reminds me why I like rebasing)

NB - one this is merged only composer.lock should need updating in main branch as composer.json points to master of this repo

Change 256377 merged by Awight:
Add option to define a separate path for the amp app

https://gerrit.wikimedia.org/r/256377

Change 266441 had a related patch set uploaded (by Awight):
Update amp and other libs

https://gerrit.wikimedia.org/r/266441

Change 266441 merged by Awight:
Update amp and other libs

https://gerrit.wikimedia.org/r/266441

Change 266443 had a related patch set uploaded (by Awight):
Point amp to temporary directory

https://gerrit.wikimedia.org/r/266443

@Eileenmcnaughton
Hooray! I tweaked the composer.lock, and verified that AMPHOME is populated correctly from our integration jobs. There's one last patch for your review,
https://gerrit.wikimedia.org/r/#/c/266443/

Also, a patch for the integration team to review:
https://gerrit.wikimedia.org/r/#/c/266447/

Change 266443 merged by Eileen:
Point amp to temporary directory

https://gerrit.wikimedia.org/r/266443

@awight am I right there is no deployment step - this qualifies as resolved.

It would be nice to finish upstreaming this https://github.com/totten/amp/pull/18 & get back to core amp - but not sure that's important at the moment

Eileenmcnaughton closed this task as Resolved.Feb 3 2016, 11:17 PM

Due to Adam's failure to complain I'm closing this

mmodell removed a subscriber: awight.Jun 22 2017, 9:44 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptJun 22 2017, 9:44 PM