-
Notifications
You must be signed in to change notification settings - Fork 4
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Execution context #276
Execution context #276
Conversation
src/ExerciseRunner/CgiRunner.php
Outdated
@@ -145,6 +166,44 @@ private function checkRequest(RequestInterface $request, string $fileName): CgiR | |||
return new Success($request); | |||
} | |||
|
|||
private function setupEnvironment(ExecutionContext $context, SolutionInterface $solution): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract this crap somewhere
src/ExerciseRunner/CgiRunner.php
Outdated
$content | ||
); | ||
} | ||
// sleep(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker tests fucked without this:
Error: "Error response from daemon: invalid mount config for type "bind": bind source path does not exist: /host_mnt/private/var/folders/gg/rgfshctn263cdzq1zhn5y9300000gn/T/php-school/3d745671"
files must take a little while to sync 🤷
src/Process/DockerProcessFactory.php
Outdated
]; | ||
} | ||
|
||
public function composer(string $solutionPath, string $composerCommand, array $composerArgs): Process | ||
{ | ||
$composerCache = System::tempDir('composer-cache'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugly af, maybe just create it during bootstrap?
src/Process/DockerProcessFactory.php
Outdated
{ | ||
while(!file_exists($environment->workingDirectory)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't work
src/Process/HostProcessFactory.php
Outdated
@@ -62,7 +65,7 @@ private function getDefaultEnv(): array | |||
return $env; | |||
} | |||
|
|||
public function phpCgi(string $solutionPath, array $env, string $content): Process | |||
public function phpCgi(Environment $environment, array $env, string $content): Process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikeymike processFactory is a bit odd for me, because we have cli & cgi runners as separate entities, but then they both depend on processFactory with specific methods for cli and cgi, so it feels like they are all tied together. Can you think of a signature for the processFactory which would generic?
@@ -38,7 +38,7 @@ public function __construct(string $file) | |||
*/ | |||
public static function fromFile(string $file): self | |||
{ | |||
return new self(InTempSolutionMapper::mapFile($file)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped this InTemp mapper now and we just do it right before executing instead, because this also fucked up docker but I can't remember how now. So I added a addFile
method to the context/env which then gets written with the solution. Seems cleaner to do it that way anyway.
No description provided.