-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fix to issue nativephp/laravel #318 + General cleanup and DRYied Publish command #97
Conversation
Rudeisnice
commented
May 31, 2024
•
edited
Loading
edited
- fixes Building for Windows on arm64 Mac fails laravel#318
- Added user interaction if no input arguments are given in build and publish commands
- DRYied publish command so it runs build command with publish option
- native:build now has option to publish
- Serving now uses native php binary
Works fine on a fresh install of Windows 11 with Laravel Herd.
Crossbuilds from a Mac works fine too. |
TODO:
|
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.
mostly looks fine, just a couple of questions
src/Commands/BuildCommand.php
Outdated
} | ||
$this->info((($publish ?? false) ? "Publishing" : 'Building') . " for {$os}"); | ||
|
||
dd("npm run {$buildCommand}:{$os}"); |
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.
Did you meant to leave this dd
in ?
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.
thats something I've missed. Probably because I was just checking for the right command, rather then building it and then forgot to remove it.
src/Commands/BuildCommand.php
Outdated
|
||
protected $signature = 'native:build {os? : The operating system to build for (all, linux, mac, win)}'; | ||
protected $availOs = ['win', 'linux', 'mac', 'all']; |
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.
Can we name this better than $availOs
- prefer full words over abbreviations
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.
I've changed variable and function names accordingly
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.
Awesome!