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

Windows support #75

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

devnote-dev
Copy link

This PR adds bindings for Duktape to Crystal on Windows, this requires changing the use of Make to a more compatible solution: I've gone with Just which is well-made and used heavily within other ecosystems like *nix. This should also work with shards install on Windows (verified locally) but there are a couple of edge-cases.

@z64
Copy link
Collaborator

z64 commented Jul 10, 2023

Especially being a 1.x shard, I don't agree with removing the Makefile - this will probably break the majority of people's builds using Duktape to no longer be able to install it without intervening to install Just in order to build this shard.

Ideally, we would use nothing that we can't assume people don't already have - make is basically guarenteed on Linux, on Windows I see most libraries (in other languages besides Crystal) simply using a batch file - which would be great.

If we want to keep both Makefile for *nix and Justfile for windows, that is fine by me, I personally don't have any stake in Windows - but we shouldn't break Linux builds.

@devnote-dev
Copy link
Author

That's fair, I hadn't considered that. At the same time, this would make the install process longer because of crystal-lang/shards#468. I guess that's not really this shard's problem though, so I'll add the Makefiles back.

@jessedoyle
Copy link
Owner

Hi @devnote-dev! First off, I just want to say that this is amazing - thanks so much for this work!

Second, I want to apologize for the radio silence. I've been on vacation the past few weeks and am just getting caught up on everything.

Haven't had great chance to review this yet, so please give me a few weeks to get this work reviewed. Rest assured, I'd love to merge any PRs that add support for Windows.

I agree with @z64 around the use of make for *nix platforms - it's the de facto build mechanism for these these platforms.

I'll give this a review over the next few weeks. Thanks again!

@@ -5,11 +5,11 @@ authors:
- Jesse Doyle <jdoyle@ualberta.ca>

scripts:
postinstall: "make -C ext clean libduktape"
postinstall: make -C clean libduktape
Copy link
Owner

@jessedoyle jessedoyle Aug 3, 2023

Choose a reason for hiding this comment

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

The -C flag changes the directory to the provided argument (ext) before running the targets.

As it currently stands, this may break the seamless postinstall for folks on non-windows platforms. Is there a way we can detect the environment from this shell command?

Copy link
Author

Choose a reason for hiding this comment

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

Ah right, yeah that should be make -C ext ....

Unfortunately there isn't, this is part of what makes Windows support difficult because Shards itself doesn't really have a concept of platform-specific script management. Some people have tried to work around this by having postinstall execute a Crystal file which handles platform-specific logic/configuration, but that may not be ideal here.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm going to have to get a VM running Windows to test this - can we invoke a shell script here instead of make directly? If so, what type of shell does it run on in Windows?

i.e.

postinstall: "./build/postinstall.sh"

Copy link
Author

Choose a reason for hiding this comment

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

Windows' shell would be command prompt (cmd) but that can't run shell scripts. The (Git) Bash interpreter also does not ship with Windows by default, so there would be no way to execute the postinstall script on Windows.

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.

3 participants