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

fix: Use binary fullname on windows. #3

Merged
merged 1 commit into from
May 10, 2020
Merged

Conversation

Et7f3
Copy link
Contributor

@Et7f3 Et7f3 commented May 10, 2020

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@Et7f3
Copy link
Contributor Author

Et7f3 commented May 10, 2020

-And .exe is bot added
+And .exe is not added

Sorry I have written too fast.

@jchavarri
Copy link
Collaborator

👍

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
Copy link
Contributor Author

Et7f3 commented May 10, 2020

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
Copy link
Contributor Author

Et7f3 commented May 10, 2020

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
Copy link
Contributor Author

Et7f3 commented May 10, 2020

Here is the commit you have 15 days to see the log before GH evict them. Et7f3@83a2ea3

@jchavarri
Copy link
Collaborator

@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.

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.

Error when running bsb -make-world + atdgen on Windows
2 participants