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

new fork to exclude external commits #85

Closed
wants to merge 2 commits into from
Closed

new fork to exclude external commits #85

wants to merge 2 commits into from

Conversation

syke99
Copy link
Contributor

@syke99 syke99 commented May 23, 2022

This should remove the commits to other files being tied to the PR like the previous PR for this fix was experiencing.

@syke99
Copy link
Contributor Author

syke99 commented May 23, 2022

This PR has been converted to a draft as I need to continue getting the example Make command targets to work with Windows. (The npm link command is failing to find the package.json. It's malforming the path, so may need to remove Make with Chocolatey and re-install another way) The bujs.ci, budjs.check, and budjs.test Make command targets may not work and will need to be tested as well.

@syke99 syke99 marked this pull request as ready for review May 23, 2022 14:31
@syke99 syke99 marked this pull request as draft May 23, 2022 14:31
…e that will be made in another PR, but should work after that
@syke99 syke99 marked this pull request as ready for review May 23, 2022 15:57
@syke99
Copy link
Contributor Author

syke99 commented May 23, 2022

It ended up not being a corrupted make issue, rather than a make-on-Windows-specific issue with needing to separate (cd example/basic && ../../livebud) into

cd example/basic
npm link
npm ../livebud

as I found out each line in a Makefile runs in its own subshell, so it wasn't looking in the right place for the top-level package.json file

Running any of the make example.* commands will still fail due to the syscall in package/socket/child.go, but I have a fix for that (that will work up to the point of v8go failing) ready to be put into a PR once this PR is approved and merged and I can fork bud again

@matthewmueller
Copy link
Contributor

matthewmueller commented May 24, 2022

Hey @syke99, thank you for the effort. I must admit, the more I reflect on these changes, the more concerned I get.

We're making the Makefile more fragile for our current target users (OSX and Linux) while trying to adapt it for a platform we currently don't support. We still have some large hills to climb in order to properly support Windows (V8 support & passing file descriptors between binaries).

As I look to Laravel, I see that they also recommend using Laravel with WSL2 and it's not clear that they support Windows native. We're far from the capabilities of Laravel and I'd like to focus our effort on catching up before expanding platforms.

What do you think? I'd still like to support native Windows at some point, it's one of the key benefits of using Go, I just don't think we have enough usage or expertise yet to add proper support for the platform.

If the above makes sense, I have some suggestions:

  • I'm open to continuing this effort if we can properly abstract these concepts in a cross-compatible way. I like our approach with mkfile_path, could we have windows-compatible functions for spawning subshells?
  • Otherwise, we add a note in the Contributing Guide for Windows users that we recommend using WSL2

Sorry if this comes as an unhappy surprise. I spent some more time thinking about it and it crossed the comfort to uncomfortable threshold 😓

@syke99
Copy link
Contributor Author

syke99 commented May 24, 2022

Hey Matt, I’m 100% okay with just adding a note in the README for users to just use WSL2. Honestly, getting native Windows development to work is going to be a HEAVY lift, especially with the v8 and the file descriptors/handlers. It’s something I’d still like to tinker with, but I’m definitely alright with not putting nearly as much focus on it right now!!

@matthewmueller
Copy link
Contributor

Sounds good, thanks @syke99. Closing this PR then, but I'd be open to adding a note in the contributor guide for native Windows users!

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.

2 participants