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

enhancement / address build command papercuts #153

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

gwleuverink
Copy link
Contributor

@gwleuverink gwleuverink commented Jan 7, 2025

This PR adresses a couple of papercuts I've found in the build/publish commands.

Fixed

  1. Composer install & cleanup env file were done before the app was copied to the build directory
    • When the build command finished, all dev dependencies were removed from the base path, breaking some things locally (and in my native:dev script)
    • When the build exits prematurely, the backup env file would not be restored

I've addressed this by porting the app copy logic to a php trait. This way the app is copied over before the electron-builder is executed. The copy step inside the electron-builder has been removed in favor for this approach. This way we don't have to modify any userland code.

  1. Files that could contain sensitive information were not removed before build

During the copy step I account for files and directories that should be skipped. I've used an internal list here, which merges with the list of globs in the nativephp config file.

  1. When building all mac architectures at once the mac-latest.yaml was overwritten by the last command

I've tried fixing that before in PR #145. Sadly this caused the last architecture not to compile with the correct php version

I've addressed this by removing the php building step as from the package.json scripts, and rather embed them in the build process by using a beforePack hook.

Other points for your consideration

  1. When GITHUB_REPO, GITHUB_OWNER, GITHUB_PRIVATE are cleaned up, the publish command cannot upload to github.

They are all caught by the GITHUB_* wildcard. I assume this assume is true for the other update providers too. Maybe the default list of env keys to be cleaned should be more specific, like: GITHUB_TOKEN? (that's the only important one that needs to be cleared anyway)

I'm not sure if this still is an issue after these changes. Since we're not directly updating the projects' main env file anymore.

  1. Logs do not rotate. File size can increase dramatically

Should we default to LOG_STACK=daily when building the app?

  1. Should we also clean the node_modules directory when building?

This can become unwieldy quick! A fresh Laravel install already comes with a 30mb node_modules directory.
Not sure if we want to keep node_modules/.bin in this case.. An argument could be made to keep that around.

  1. The x64 mac build is packaged with a x86 php binary

Not sure if this is by design. php.js was invoked with the --x86 flag, while electron builder was invoked with --mac --x64 in the build:mac-x86 script.

I kept this behaviour in, but not sure why it is like this

@gwleuverink gwleuverink marked this pull request as ready for review January 8, 2025 13:13
@gwleuverink
Copy link
Contributor Author

I'm sure this needs more polish, but I'd appreciate a review since I'm changing around quite a bit of stuff @simonhamp

Don't want to make a wrong turn & have to backtrack. Appreciate it! 🙏🏻

@gwleuverink
Copy link
Contributor Author

gwleuverink commented Jan 8, 2025

Tests didn't pass for php 8.1 because I used some some incompatible syntax

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.

1 participant