-
Notifications
You must be signed in to change notification settings - Fork 131
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
Interest in Linux and macOS npm
bins being "normal?"
#735
Comments
Thanks for opening this @joneshf! I'm certainly open to discuss the matter, and even merge a patch for this. |
I think this is also an issue for the purescript npm installer, and I think it was originally a deliberate decision to use native binaries where possible because having everything happen inside a useless nodejs process does cause some weirdness, especially with the repl: for example, there would be cases where it would just exit randomly with a message that said something along the lines of "Broken pipe". There may be discussion of this in the compiler issue tracker or the npm-installer issue tracker somewhere. |
I hear that and respect it. I can't promise that I'll be able to address all issues, but yes feel free to ping me if anything comes up around this and I'll do what I can to help. One option is to add a secondary {
…
"bin": {
"spago": "./spago",
"spago_node": "./spago.js"
},
…
} Where #!/usr/bin/env node
const cp = require("child_process");
const path = require("path");
// Ignore SIGINT (Ctrl-C) because we don't want to terminate this process from
// it (#483). The actual spago binary will handle it for us
process.on('SIGINT', () => {});
const spagoFilename = { win32: "spago.exe" }[process.platform] || "spago";
const spago = cp.spawn(path.join(__dirname, spagoFilename), process.argv.slice(2), {stdio: 'inherit'});
spago.on('error', (err) => {
console.log("Downloading the spago binary failed. Please try reinstalling the spago npm package.");
}); People can opt-in to using a "normal" |
@f-f Any thoughts on my previous comment? |
fwiw I would be against making this same change in the compiler npm installer, for the reason I gave in my previous comment. I think it can be argued that tooling that assumes that npm bins are JS files is incorrect; as I understand it, it's not all that uncommon for other npm packages to use non-JS bins. See also #564 and yarnpkg/berry#882 |
@joneshf sorry, I somehow missed the notification on this one - thank you for the ping. |
No worries. It shouldn't show up on the path. It should be like any other |
Oh, I see that the recommended install is with If an additional binary showing up on the path is a deal-breaker, no worries (we can close this). If a different name would make it not a deal-breaker, I'm down for whatever name. |
I'm also fine with any name, as long as it doesn't start with I also now wonder if this is worth it - is this broken and this is a fine fix, or are we better off documenting a workaround? |
Yeah, I dunno. I remember doing some research about "normal" vs. native |
I think what I meant to say was: are you able to use any workaround for this or is it just broken and there's no workaround? |
Oh sorry, I think I see. The only workaround I can think of would be someone maintaining another If there's some configuration or |
Right - I am not very excited about having another executable installed, so I think a new npm package would be the best way forward here. I would not mind publishing it from here too, so that it stays in sync with the main one |
Fair enough. What's the next step here, then? |
@joneshf I'd merge a PR that:
|
The new Spago has a "normal" NPM package, as it's just JS |
Hey thanks for following up on this! Sorry for not doing anything about this issue for the past…2+ years. Looks like |
Time flies 🙂 I hope you're doing well! |
Issue
According to this comment:
spago/npm/spago
Lines 2 to 3 in 960a310
npm
package's bin is a "normal"1npm
bin on Windows only. On Linux and macOS, it appears to be replaced by the underlying nativespago
binary. Using a native binary (and especially using a native binary only on certain platforms) breaks other tooling that expects annpm
package to provide a "normal"npm
bin (on all platforms).An example of tooling that breaks with a native binary is
rules_nodejs
's Generated macros for npm packages with bin entries. If you're unfamiliar withbazel
jargon, the long and short is that thisbazel
tooling that's specific to buildingnode.js
projects will make helpers under the assumption that any binaries in annpm
package are "normal"npm
bins. This works for free for anynpm
package so long as it makes a "normal"npm
bin.Question
Would you be willing to have a valid
npm
bin on all three supported platforms? The Linux and macOS platforms are what would require some changes. If you're interested, we can discuss more what some options are. But if you're not, we can close this issue.1: I say "normal" only because I don't know what else to call an
npm
package's bin that works with other JS tooling vs. one that doesn't really. If there's already a defined word for this sort of thing, I'm happy to switch to saying that instead.The text was updated successfully, but these errors were encountered: