Skip to content
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

Merged
merged 12 commits into from
Jul 18, 2024

Conversation

Rudeisnice
Copy link
Contributor

@Rudeisnice Rudeisnice commented May 31, 2024

  • 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

@Rudeisnice
Copy link
Contributor Author

Rudeisnice commented May 31, 2024

Works fine on a fresh install of Windows 11 with Laravel Herd.
Windows 11 Config

  • Enabled Developer Mode
  • Set-ExecutionPolicy RemoteSigned (For Laravel Herd npm )

Crossbuilds from a Mac works fine too.

@Rudeisnice Rudeisnice marked this pull request as ready for review June 1, 2024 11:35
@Rudeisnice Rudeisnice changed the title Fix to issue nativephp/laravel #318 + General File cleanup and DRYied the whole Publish command Fix to issue nativephp/laravel #318 + General File cleanup and DRYied Publish command Jun 1, 2024
@Rudeisnice Rudeisnice changed the title Fix to issue nativephp/laravel #318 + General File cleanup and DRYied Publish command Fix to issue nativephp/laravel #318 + General cleanup and DRYied Publish command Jun 1, 2024
resources/js/package.json Outdated Show resolved Hide resolved
@Rudeisnice Rudeisnice marked this pull request as draft June 12, 2024 16:06
@Rudeisnice Rudeisnice marked this pull request as ready for review June 17, 2024 09:49
@Rudeisnice Rudeisnice marked this pull request as draft June 21, 2024 12:28
@Rudeisnice
Copy link
Contributor Author

TODO:

  • fix --no-interaction

@Rudeisnice Rudeisnice marked this pull request as ready for review June 21, 2024 13:49
Copy link
Member

@simonhamp simonhamp left a 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

}
$this->info((($publish ?? false) ? "Publishing" : 'Building') . " for {$os}");

dd("npm run {$buildCommand}:{$os}");
Copy link
Member

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 ?

Copy link
Contributor Author

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.


protected $signature = 'native:build {os? : The operating system to build for (all, linux, mac, win)}';
protected $availOs = ['win', 'linux', 'mac', 'all'];
Copy link
Member

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

Copy link
Contributor Author

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

resources/js/package.json Outdated Show resolved Hide resolved
Copy link
Member

@simonhamp simonhamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@simonhamp simonhamp merged commit ab99e94 into NativePHP:main Jul 18, 2024
1 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building for Windows on arm64 Mac fails
2 participants