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

Add support for windows #4

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Conversation

dworthen
Copy link
Contributor

  • Reimplement the logic in https://just.systems/install.sh for native JavaScript installation, removing the dependency on a specific shell.
  • Add support for windows
  • Add a proxy bin runner (index.js) due to this issue, [BUG] Preinstall script runs after installing dependencies npm/cli#2660 that prevents dynamically changing the bin field preinstall. Alternatively could provide two different bin names.
  • Expose the installation script as a bin so that users may upgrade to newer releases of just anytime by running just-install
  • Bump minimum Node engine to 18.9 due to dependency on os.machine.

Resolves #2

- Reimplement the logic in https://just.systems/install.sh for native
JavaScript installation, removing the dependency on a specific shell.
- Add support for windows
- Add a proxy bin runner (index.js) due to this issue,
npm/cli#2660 that prevents dynamically
changing the bin field preinstall. Alternatively could provide two
different bin names.
- Expose the installation script as a bin so that users may upgrade to
newer releases of just anytime by running just-install
- Bump minimum Node engine to 18.9 due to dependency on os.machine.
@brombal
Copy link
Owner

brombal commented Apr 30, 2023

@dworthen Thanks a lot for this! I haven't been able to add Windows support yet because I don't regularly work with it, so I am going to (finally) setup a Windows box to review this.

I will say my immediate thoughts are that my goal for this utility was to simply be a thin wrapper around Just's install.sh script. I didn't want to reinvent the wheel or be too dependent on Just's publishing strategy, because that could make it too fragile. Just already claims you can use their install.sh on for Windows (and makes specific mention that without sh you're going to need special setup, and therefore maybe you're just not a good candidate for just-install), so I had thought that implementing this for Windows would not need to involve writing a "native" JS install script. Granted, all of that is based on my limited understanding of how shell scripts work in Windows, so it may not be true and is something I will play around with.

That being said, I don't mean to discourage your PR, and if this is really the best approach for windows users, then that's what it is. As I mentioned I will play around with it and see.

@dworthen
Copy link
Contributor Author

dworthen commented May 1, 2023

Hi @brombal, thanks for your feedback and I look forward to your investigation and on your decision.

Since you brought up not having much familiarity with scripting on Windows I thought I would provide some information on the topic in hopes it saves you some troubles and helps out. Though Just claims the install.sh script works on Windows, it only does so with the caveat of requiring a bash emulation shell in order to run the script, something like git-bash or cygwin. Looking at the code of install.sh it looks like it specifically relies on git-bash. Bash scripts do not work on Windows and are flaky at best when using emulations as many of the emulations don't provide full parity with bash commands. So if you are working on setting up a Windows environment and would like to test the installation process using the install.sh script then know that you will need to run it using git-bash and not powershell or cmd. Hope this helps in your research.

Let me know if you have any questions or would like me to test something on Windows for you.

@brombal
Copy link
Owner

brombal commented May 9, 2023

@dworthen Thanks again for this. I did a bunch of testing on it (in large part just so I could learn the nuances of how Windows handles this). It was tricky and I decided to change the direction of the solution a bit, but your code was a big help to get me through it.

My concern with re-implementing Just's installation script turned out to be more than just theoretical—the installation failed for me on my own laptop because this revision didn't have the correct label for "arm64-Darwin" (M1 macbooks). This is the scenario I'm trying to avoid; I don't want to have to keep just-install up to date with whatever changes the Just developers make (I know it was probably just an oversight here, but I think the point is still valid).

That being said, I don't see any way around taking this approach for Windows, since even Just's install script doesn't work out-of-the-box on Windows. So what I settled on instead was a hybrid install script that downloads directly from GitHub for Windows, and uses the Just install.sh script for other platforms. It's a little more frankenstein-y than I would like, but I think it better satisfies my previous desire to use Just's install script whenever possible.

The other issue of course is that Windows requires the binary to end in .exe. And since the "bin" field in the package.json file needs to be the same for both platforms, the only solution was to pass the execution through a node script with child_process, as you had it. Since yarn and now Windows users need to rely on this strategy anyway, I decided to just make it work this way for everyone. This removed the need to swap files around for yarn, so overall it reduced the complexity a bit, which is nice.

Anyways, it should be working in Windows if you want to try it, and if you have any feedback please let me know.

@SilenLoc
Copy link

any progress here?

@brombal
Copy link
Owner

brombal commented Feb 1, 2024

@SilenLoc hey, yes I was able to get it working on windows but left it on a branch awaiting help to review. The other contributor did not respond though, so it went stale. I will revisit it in the next day or so and merge it into a new major version. I'll keep you posted!

@SilenLoc
Copy link

SilenLoc commented Feb 1, 2024

@SilenLoc hey, yes I was able to get it working on windows but left it on a branch awaiting help to review. The other contributor did not respond though, so it went stale. I will revisit it in the next day or so and merge it into a new major version. I'll keep you posted!

Thank you for your answer :)

@brombal brombal merged commit 86c1848 into brombal:main Feb 2, 2024
@brombal
Copy link
Owner

brombal commented Feb 2, 2024

Hi @SilenLoc, I just published version 2.0.0 with Windows support. If you'd like to try it out, and let me know if you have any problems. Thanks!

@SilenLoc
Copy link

SilenLoc commented Feb 4, 2024

@brombal looks good :)

image

tested on windows:

image

@brombal
Copy link
Owner

brombal commented Feb 4, 2024

@SilenLoc Awesome, thanks!

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.

Will this work on Windows?
3 participants