fix: resolve Phar updater strategies #481
Conversation
|
@owenvoke you need to rebase. |
This makes sure that the Phar name is set for the GitHub Releases strategy.
owenvoke
force-pushed
the
feature/phar-updater
branch
from
477b05c
to
99025c9
Compare
|
@nunomaduro, done. 👍🏻 I have only tested these changes on the repo in the issue, so not sure if we want to do some further testing. 🤷🏻 |
|
Any news about this? What is preventing the merge? |
|
@sergix44, I think it was just needing further testing (on real-world projects). If you have a project, feel free to test this branch. 🙂 |
Uses laravel-zero/framework#481 to test if it can work for production
| @@ -67,19 +69,27 @@ public function register(): void | |||
| $this->app->singleton(Updater::class, function () use ($build) { | |||
| $updater = new PharUpdater($build->getPath(), false, PharUpdater::STRATEGY_GITHUB); | |||
|
|
|||
| $composer = json_decode(file_get_contents(base_path('composer.json')), true); | |||
| $name = $composer['name']; | |||
| $composer = json_decode(file_get_contents($this->app->basePath('composer.json')), true); | |||
There was a problem hiding this comment.
I think there should be a way to set the package name without Composer, as there is no real guarantee that the compiled binary is run in a directory with a composer.json file, nor do I think there should need to be?
There was a problem hiding this comment.
This is the composer.json within the Phar file. It should always exist.
There was a problem hiding this comment.
Okay, that's weird, because for me, it tries to load it from the working directory.
hydephp/cli@925f00b It could be due to how my paths are setup though, in which case this is my fault. Gonna check it out.
There was a problem hiding this comment.
Yeah, this is my bad. My application sets the working base path to be the working directory. For my usages, I would need this to be an absolute path, but that is probably not something Zero should worry about.
There was a problem hiding this comment.
Update: Changing the line to the following works better for me.
$composer = json_decode(file_get_contents(__DIR__.'/../../../../../../composer.json'), true);
Testing this branch now (hydephp/cli@4e2817a), but I'm erroring on this version. It tries to download https://github.com/hydephp/cli/raw/v0.5.2/hyde/builds/hyde instead of https://github.com/hydephp/cli/raw/v0.5.2/builds/hyde Edit: I can at least confirm that this branch makes so I don't get the following error: So this should at least fix laravel-zero/laravel-zero#193 (comment) |
Mine is also a real world project, so 🤷 And then set it as the update strategy: config/updater.php Now the self update from the github releases should work as expected |
|
Okay so I've done some more testing, after changing the Composer include to use a base path (see review comment) it seems like this works, in that the release is downloaded. I still get a Phar corruption error during the installation But what is weird is that the downloaded binary works when I run it, so something must be wrong when verifying the download?
|
This depends on #480 to be merged first. 👍🏻 See 477b05c for the actual changes.
What does this do:
updater.phar_nameconfig key, that, when set, will be used to set the name of the Phar file as stored on GitHub, GitLab, etc.Updater/ProviderThis solution appears to work for resolving the mentioned issue, however it could probably do with some testing elsewhere to ensure it doesn't break any existing applications.
Closes laravel-zero/laravel-zero#459