Develop command working on Windows again #47

chrisreedio · 2023-07-28T01:14:54Z

The latest build had broken the develop command on Windows for @fadeaway and myself.
The develop command should take the current OS / arch into account since it will be running locally on that system.
The build scripts pass in flags to override the target platform/arch.

Added single quote to editor config

Currently on Windows we the native:serve works everytime and the build is 'working' with no errors but we never see a dist directory or output files.

That was the case before the work in this PR so there must still be something going on.

The prior work had broken the develop command on Windows. The develop command should take the current OS / arch into account since it will be running locally on that system. The build scripts pass in flags to override the target platform/arch. * Added single quote to editor config

chrisreedio · 2023-07-28T01:38:48Z

.editorconfig

@@ -7,6 +7,8 @@ indent_size = 4
 indent_style = space
 insert_final_newline = true
 trim_trailing_whitespace = true
+quote_type = single


VSCode kept wanting to put double quotes which was against the current style of this repo so I added this until we have styleci setup

chrisreedio · 2023-07-28T01:39:20Z

.editorconfig

@@ -7,6 +7,8 @@ indent_size = 4
 indent_style = space
 insert_final_newline = true
 trim_trailing_whitespace = true
+quote_type = single
+max_line_length = 120


This may also be controversial but again, it kept most lines as they were and until we get styleci setup, it should help.

chrisreedio · 2023-07-28T01:40:04Z

resources/js/electron-builder.js

 let phpBinaryFilename = 'php';
-if (isWindows) {
+// Default to the current platform for develop mode. Build will pass in an arg that overrides this


Aligns the target with the directory structure.

chrisreedio · 2023-07-28T01:40:26Z

resources/js/electron-builder.js

 let phpBinaryFilename = 'php';
-if (isWindows) {
+// Default to the current platform for develop mode. Build will pass in an arg that overrides this
+let targetOs = process.platform;


Set the target to the current OS, if building, an override flag will be sent in.

Garanaw · 2023-07-28T17:31:43Z

resources/js/electron-builder.js

    targetOs = 'win';
    phpBinaryFilename += '.exe';
-}
-if (isLinux) {
+} else if (isLinux) {


These else-if blocks seem redundant now (not the first one as it's setting the binary filename)

The switch normalizes the targetOs from process.platform, the if statements handle the override flags passed in during build stages.

@chrisreedio Just note when it is Linux, but also Win32... at the same time. (Ubuntu in WSL in Windows)

https://github.com/NativePHP/electron/issues/50

I don't believe you'd get that as the process.platform will always be the actively running platform. So if running it in WSL, it'd be Linux and if running in powershell (et al) it'd be win32 which gets converted to 'win' for the bin directory lookup.

I run WSL in Windows 10 and have been working with NativePHP from Windows itself. In the long run, I'd prefer to work in WSL and build for Windows.

Note that only Windows 11 allows graphical WSL (linux apps). If in windows 10, the native:develop command has to be run from Windows (not WSL (for now)).

Could you rename the variables in this case?
I think isWindows, isLinux etc is confusing. It's more like "forcedOS" right? Maybe we can simplify this a bit.

chrisreedio · 2023-07-29T12:22:15Z

I agree on all points. Was thinking the same while working on it but didn't want to drive a bulldozer through that yet but after thinking on it a day or two, it can definitely be simplified. I'll get this done later today or first thing tomorrow.

…

On Sat, Jul 29, 2023, 4:08 AM Marcel Pociot ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In resources/js/electron-builder.js <#47 (comment)>: > targetOs = 'win'; phpBinaryFilename += '.exe'; -} -if (isLinux) { +} else if (isLinux) { Could you rename the variables in this case? I think isWindows, isLinux etc is confusing. It's more like "forcedOS" right? Maybe we can simplify this a bit. — Reply to this email directly, view it on GitHub <#47 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASQMGKH2SUAIPCB4L527GNLXSTHHHANCNFSM6AAAAAA222P6HU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mpociot · 2023-07-29T09:08:43Z

resources/js/electron-builder.js

    binaryArch = 'x64';
+} else if (isDarwin) {
+    binaryArch = 'x86';


I'll update the php-bin package so that the mac binaries are all in an x64 folder. I think it was just an error on my end and it should be like that anyway. Then we can get rid of this macOS specific check.

StoneyEagle · 2023-07-30T16:32:58Z

I got to get the Electron app to install the php dependencies.
By prefixing them with npx and node, windows will properly execute the scripts and not open the electron-builder.js file.

vendor\nativephp\electron\resources\js\package.json

{
  "scripts": {
    "dev": "npx electron-vite dev --watch",
    "postinstall": "node electron-builder install-app-deps",
}

chrisreedio · 2023-07-31T07:33:43Z

I got some more done on this today but had some life stuff and wasn't able to get finished. Will do my best to get the rework on this pushed by end of day for me tomorrow. (around 24h from now)

Also added some styling config to editor config that matches the existing code style. This of course will be a moot point once have style CI setup. * Added single quote to editor config * Building for windows in the develop command. * minor logic corrections * Fixed platform names * Tidied up the build target selection code in electron-builder.js

chrisreedio · 2023-08-01T07:53:28Z

I saw this as a merge conflict in the yarn.lock

<<<<<<< main
"@esbuild/win32-x64@0.18.17":
  version "0.18.17"
  resolved "https://registry.npmjs.org/@esbuild/win32-x64/-/win32-x64-0.18.17.tgz"
  integrity sha512-fGEb8f2BSA3CW7riJVurug65ACLuQAzKq0SSqkY2b2yHHH0MzDfbLyKIGzHwOI/gkHcxM/leuSW6D5w/LMNitA==
=======
"@esbuild/darwin-arm64@0.18.17":
  version "0.18.17"
  resolved "https://registry.npmjs.org/@esbuild/darwin-arm64/-/darwin-arm64-0.18.17.tgz"
  integrity sha512-M9uJ9VSB1oli2BE/dJs3zVr9kcCBBsE883prage1NWz6pBS++1oNn/7soPNS3+1DGj0FrkSvnED4Bmlu1VAE9g==
>>>>>>> main

I went with the Darwin one to be safe but I don't remember swapping this out.

Maybe @mpociot or someone can advise and double check me on this?

I still have yet to get the build command on Windows to create a dist directory much less place any output files into it.

@fadeaway looked at this at length too and was unsure how to get it working.

I can can revisit this in the coming days. This at least gets the development working well within Windows itself (not WSL).

resources/js/electron-builder.js

Removed added space from merge conflict.

CharlieEtienne · 2023-08-19T19:56:12Z

Hi, I think I get this behaviour on Windows (no dist folder created and no .exe). See #177

I tried to replace my files with these commits changes, but I still don't have a dist folder after building.

I also tried different fixes found on other issues, but nothing does the trick for my case...

mpociot · 2023-10-18T10:07:04Z

Alright...I just checked this PR again and the latest changes fixed the develop command on Windows already which is why I'm closing this.
Thank you for your PR!

chrisreedio commented Jul 28, 2023

View reviewed changes

Garanaw reviewed Jul 28, 2023

View reviewed changes

chrisreedio mentioned this pull request Jul 29, 2023

Update electron-builder.js #52

Merged

RoccoHoward mentioned this pull request Jul 29, 2023

[Bug]: A JavaScript error occurred in the main process NativePHP/laravel#131

Closed

ShaneShipston mentioned this pull request Jul 29, 2023

Resolve Windows opening config file in default application #60

Merged

mpociot reviewed Jul 29, 2023

View reviewed changes

mpociot mentioned this pull request Jul 29, 2023

Linux extra condition for electron builder. #57

Merged

chrisreedio added 2 commits August 1, 2023 02:48

Merge branch 'main' into main

cc376c9

chrisreedio commented Aug 1, 2023

View reviewed changes

resources/js/electron-builder.js Show resolved Hide resolved

chrisreedio added 2 commits August 1, 2023 02:55

Update yarn.lock

e5838a4

Removed added space from merge conflict.

Fixed whitspace in yarn.lock

b090306

mpociot closed this Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop command working on Windows again #47

Develop command working on Windows again #47

chrisreedio commented Jul 28, 2023

chrisreedio Jul 28, 2023

chrisreedio Jul 28, 2023

chrisreedio Jul 28, 2023

chrisreedio Jul 28, 2023

Garanaw Jul 28, 2023

chrisreedio Jul 28, 2023

RoccoHoward Jul 29, 2023

chrisreedio Jul 29, 2023

chrisreedio Jul 29, 2023

mpociot Jul 29, 2023

chrisreedio commented Jul 29, 2023 via email

mpociot Jul 29, 2023

StoneyEagle commented Jul 30, 2023

chrisreedio commented Jul 31, 2023

chrisreedio commented Aug 1, 2023

CharlieEtienne commented Aug 19, 2023

mpociot commented Oct 18, 2023

Develop command working on Windows again #47

Develop command working on Windows again #47

Conversation

chrisreedio commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisreedio commented Jul 29, 2023 via email

Choose a reason for hiding this comment

StoneyEagle commented Jul 30, 2023

chrisreedio commented Jul 31, 2023

chrisreedio commented Aug 1, 2023

CharlieEtienne commented Aug 19, 2023

mpociot commented Oct 18, 2023