Add command to build standalone binary #260
Conversation
6907e99 to
34ab073
Compare
lyrixx
left a comment
There was a problem hiding this comment.
Love it 👍🏼 ❤️
I'll need to try this locally, but this is a very good start!
| timeout: 5 * 60 | ||
| ); | ||
| $io->text('Running command: ' . $installSPCDepsProcess->getCommandLine()); | ||
| $installSPCDepsProcess->mustRun(fn ($type, $buffer) => print ($buffer)); |
There was a problem hiding this comment.
tty must be enabled, to interact with the process?
There was a problem hiding this comment.
Nope, no need to interact with the sub processes. This line is just copied from the https://github.com/jolicode/castor/blob/main/src/Console/Command/RepackCommand.php#L109 to output the subprocess output. Is there a more appropriate way to do this?
There was a problem hiding this comment.
The last time I run the doctor cmd, it asked me few things. IIRC, "do you want to install XXX", and then type my password
There was a problem hiding this comment.
This is intriguing, it's passing the CI test with no issues. I'll delete the build tools on my computer and then attempt to run compile to see what needs to be fixed.
|
You'll also need to
|
b48fdc1 to
d8b7ee4
Compare
|
@lyrixx I have implemented your feedbacks and pushed a single commit to ease the review. I'll squash and rebase before merge. |
lyrixx
left a comment
There was a problem hiding this comment.
Yeah! Except two little nitpick, it's okay for me!
I'll need to test it locally before the merge. I'll try this ASAP.
ae3e176 to
387b186
Compare
|
I realized something, since you "pack" the application before compiling it, it's not possible to edit the PHP code anymore, right? Could it be a drawback? For example, we (as castor team) could not distribute castor as a static binary? If I'm right, is it possible to make the packing opt-in? |
|
You're right, I'll explore ways to elegantly handle both cases in a single command. Since one requires to run from vendor/bin and the other from the phar archive. Also that would make all the repacking options superfluous for this case. Since both behaviors end up merging a custom built php and a phar archive, I'm wondering if the compile command responsibility could be just that. Just passing php build options and the phar path, whether it's a repacked castor app or the castor tool itself. But I liked the simplicity of having both the repack and compile in a single command. Not sure I would want to lose that and force users to run both commands separately. |
|
I let you explore 💛 |
|
I merged #265 ; Now we test all examples (all tests actually) with a specific binary
So you can add your compile binary to the list :) |
2287288 to
971bc6b
Compare
|
Some updates: I've decided to simplify the This means it will now be able to support compiling the castor phar tool or a repacked phar castor apps. I've thoroughly documented the process for compiling a repacked phar castor app in the docs, so for those who previously needed to do two steps instead of one, it won't be a huge obstacle. I've also updated the CI to run the test suite with the compiled binary, and it revealed some confusing issues that I still need to understand how to fix. This seems to only happen in a phpunit environment. When I run And running: I'll try to investigate more deeply soon. |
971bc6b to
48a389e
Compare
1d39713 to
7718423
Compare
|
My previous issue was related to tests running tasks with I've fixed it by introducing a The remaining issue is a mistery to me When On compiled version: On PHAR and bin/castor version: Any ideas where it could come from ? If no quick solutions comes up I suggest we support both outputs in tests and ship it because I don't think it's worth blocking the feature as the whole IMO . @lyrixx WDYT ? |
lyrixx
left a comment
There was a problem hiding this comment.
I'll review the rest of the code later.
About the issue, I may have a lead. The SAPI is micro, can you try to change it to cli ? Because ATM, it breaks fews things, like call to dump() (it outputs in HTML format)
Yes, found it. With the static binary: proc_open('echo 1 >/dev/null', [['pty'], ['pty'], ['pty']], $pipes)IMHO, this issue should be reported upstream (I'll do 👍🏼 - see crazywhalecc/static-php-cli#335) In the meantime, I'll perpare a PR to make the test more stable 👍🏼 (#276) |
lyrixx
left a comment
There was a problem hiding this comment.
This is very good 👏🏼 👏🏼 👏🏼 👏🏼
I love it.
1f9f557 to
28e5836
Compare
|
Did you search for the sapi name? Do you think I should report it upstream? |
@lyrixx This is tricky because from the project perspective I think it's reasonable to define the SAPI as Thinking about it there's 3 ways to "solve" this:
|
|
@lyrixx just discovered the |
|
Awesome 🎉 ! Did you notice the failing test? |
@lyrixx yes i'm working on a better way to generate a cache key from the build options so that the build made to generate the compiled binary will not use the cache from the |
ec3f910 to
846d265
Compare
7f03f8a to
3d45dc6
Compare
|
I've introduced a method to generate a cache key based on php build options and the command file hash: That allows to reuse download or built artifacts when no build options change. I've also updated the CI worfklow to cache those artifacts. It was a litlle bit tricky but hopefully comments are enough to explain. |
| # https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache | ||
| key: dynamic-key-${{ github.run_id }} | ||
| restore-keys: | | ||
| php-static-building-artifacts-cache- |
There was a problem hiding this comment.
I don't understand this part, It could be written:
| # https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache | |
| key: dynamic-key-${{ github.run_id }} | |
| restore-keys: | | |
| php-static-building-artifacts-cache- | |
| key: php-static-building-artifacts-cache |
More over in the Save PHP static building artifacts cache the key could be only php-static-building-artifacts-cache`
There was a problem hiding this comment.
This was my initial idea, unfortunately it doesn't work as expected.
When the restore action with key: X is executed, it looks for a cache[X] and use it (hit) if it exists.
When the save action with key: X is executed, it tries to save cache[X] but if fails if it already exists, it doesn't update the cache key.
And that's not what we want because when a new arg is added to the build command, a new directory is created: /home/runner/.cache/castor/castor-php-static-compiler/{new_compile_command_build_hash}/* and we want to update the cache with this new build artifacts.
If the current config what's happening is:
Run 1
- Restore Action:
- Does dynamic-key-${{ github.run_id }} cache hit ? No
- Does php-static-building-artifacts-cache-* cache hit ? No
- PHP Build
- Generate artifacts in
/home/runner/.cache/castor/castor-php-static-compiler/{compiler_test_command}/* - Generate artifacts in
/home/runner/.cache/castor/castor-php-static-compiler/{build_compiler_for_CASTOR_BIN}/*
- Generate artifacts in
- Save Action:
- Save
/home/runner/.cache/castor/castor-php-static-compiler/*askey: php-static-building-artifacts-cache-XXXwhere XXX is hash(castor-php-static-compiler/*)
- Save
Run 2
- Restore Action:
- Does dynamic-key-${{ github.run_id }} cache hit ? No
- Does php-static-building-artifacts-cache-* cache hit ? Yes it founds php-static-building-artifacts-cache-XXX
- PHP Build
- Use artifacts cached in
/home/runner/.cache/castor/castor-php-static-compiler/**/*, no build
- Use artifacts cached in
- Save Action:
- Tries to save
/home/runner/.cache/castor/castor-php-static-compiler/*askey: php-static-building-artifacts-cache-XXXbut fails. It's ok no new artifacts were generated anyway.
- Tries to save
Run 3, new compilation is added or options of existing build are changed in CompileCommandTest for example:
- Restore Action:
- Does dynamic-key-${{ github.run_id }} cache hit ? No
- Does php-static-building-artifacts-cache-* cache hit ? Yes, restore from Run 1
- PHP Build
- Build hash is different than Run 1, generate artifacts in
/home/runner/.cache/castor/castor-php-static-compiler/{compiler_test_command}/* - Build hash is same as Run 1, reuse artifacts in
/home/runner/.cache/castor/castor-php-static-compiler/{build_compiler_for_CASTOR_BIN}/*
- Build hash is different than Run 1, generate artifacts in
- Save Action:
- Save
/home/runner/.cache/castor/castor-php-static-compiler/*askey: php-static-building-artifacts-cache-YYYwhere YYY is a new hash ofcastor-php-static-compiler/*because new artifacts where generated since XXX
- Save
There was a problem hiding this comment.
There are two folders
- Generate artifacts in /home/runner/.cache/castor/castor-php-static-compiler/{compiler_test_command}/*
- Generate artifacts in /home/runner/.cache/castor/castor-php-static-compiler/{build_compiler_for_CASTOR_BIN}/*
because
- there is one for castor itself?
- there is another one for tests/CompileCommandTest.php
If yes, I almost got it, but not perfectly :)
If not I'm totally lost 😅
Anyway, I'll approve the PR, play a bit with it, with the cache, etc :)
Thanks
There was a problem hiding this comment.
Yes exactly! Both build are not using the same extensions, the one in the test has less extension, it just makes sure it compiles and can run a hello world.
The compiled castor has more extensions because it needs to rune the whole suit of examples.
| # https://github.com/actions/cache/blob/main/tips-and-workarounds.md#update-a-cache | ||
| key: dynamic-key-${{ github.run_id }} | ||
| restore-keys: | | ||
| php-static-building-artifacts-cache- |
There was a problem hiding this comment.
There are two folders
- Generate artifacts in /home/runner/.cache/castor/castor-php-static-compiler/{compiler_test_command}/*
- Generate artifacts in /home/runner/.cache/castor/castor-php-static-compiler/{build_compiler_for_CASTOR_BIN}/*
because
- there is one for castor itself?
- there is another one for tests/CompileCommandTest.php
If yes, I almost got it, but not perfectly :)
If not I'm totally lost 😅
Anyway, I'll approve the PR, play a bit with it, with the cache, etc :)
Thanks
pyrech
left a comment
There was a problem hiding this comment.
Thanks a lot for the hard work. I probably don't understand every details, but it looks good to me in this state. Let's merge it and play with it 💛 🎉
|
thanks a lot for the hard work! 💛 I'm merging this PR, and I also prepare another one fix few things :) |
Fixes #257
I've added a
compilecommand, similar torepack. Because compiling statically also means repacking the app, I've reused therepackcommand options intocompileand set it to automatically run at the start of the compilation process.All the files needed to build the final binary are stored in
/tmp/castor-php-static-compilerfolder and cached in the CI with a cache key based onsrc/Console/Command/CompileCommand.phpandtests/CompileCommandTest.php.Since this process includes downloading and building PHP source, it's no surprise that the test suite runs 3 minutes slower when cache is not warm. Fortunately, it will happen only when
src/Console/Command/CompileCommand.phportests/CompileCommandTest.phpis modified and thus have a very minimal impact overall.Here's the
compile -houtput for reference:Here's a cold
php vendor/bin/castor compilerun output example:Details
TODO: