TheSpyder / rescript-atdgen-generator

Pre-compiled versions of atdgen binaries to be consumed from ReScript projects
Other
10 stars 4 forks source link

fix: Use binary fullname on windows. #3

Closed Et7f3 closed 4 years ago

Et7f3 commented 4 years ago

Fix #1 Ninja use CreateProcess

And .exe is bot added

This parameter must include the file name extension; no default extension is assumed.

Also npx atdgen.exe doesn't work so we have to specify the fullname. I have to use ../../ because ninja is executed inside lib/bs

jchavarri commented 4 years ago

Thanks @Et7f3 ! this is awesome.

And .exe is bot added

What do you mean with this? I would prefer to keep the npx or yarn run approaches as they abstract over the shell where bsb is running while the approach in this PR only works with bash but it'll probably break with powershell? 🤔

I wonder if changing the bin field in package.json would help?

https://github.com/jchavarri/bs-atdgen-generator/blob/44467620802699e278fd5a42d1054612e8f8629d/package.json#L17

  "bin": {
-    "atdgen": "./atdgen.exe"
+    "atdgen.exe": "./atdgen.exe"
  },
jchavarri commented 4 years ago

In any case, this fixes the issue, we can investigate in other PRs. Thanks again!

Et7f3 commented 4 years ago
-And .exe is bot added
+And .exe is not added

Sorry I have written too fast.

jchavarri commented 4 years ago

👍

By the way, I saw there is another way to call atdgen from windows which doesn't require the full path. In bsconfig.json:

-      "command": "npx atdgen -bs $in"
+      "command": "cmd /c npx atdgen -bs $in"
Et7f3 commented 4 years ago

CreateProcess is a winapi call that ninja use after pasing build.ninja so it is not a issue with shell because they won't parse this command they just call ninja. For the sake of safety I have checked with a commit and it seem clean.

Et7f3 commented 4 years ago

Have you tried to use the tricks with npm to expose both name: because here you run first cmd then npx then atdgen... and cmd isn't available on linux.

Et7f3 commented 4 years ago

Here is the commit you have 15 days to see the log before GH evict them. https://github.com/Et7f3/bs-atdgen-generator/commit/83a2ea3f4464fa1dde84a3049aecf3693576d183

jchavarri commented 4 years ago

@Et7f3 Thanks.

cmd isn't available on linux.

I know... but I want a way for users to use the generator without having to think about relative paths etc.

I documented both ways to work in Windows in #4.