spago icon indicating copy to clipboard operation
spago copied to clipboard

Interest in Linux and macOS `npm` bins being "normal?"

Open joneshf opened this issue 4 years ago • 15 comments

Issue

According to this comment: https://github.com/purescript/spago/blob/960a310d6efca3bb40009eb06d88382e4670ccef/npm/spago#L2-L3 it appears that the npm package's bin is a "normal"1 npm bin on Windows only. On Linux and macOS, it appears to be replaced by the underlying native spago binary. Using a native binary (and especially using a native binary only on certain platforms) breaks other tooling that expects an npm 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 with bazel jargon, the long and short is that this bazel tooling that's specific to building node.js projects will make helpers under the assumption that any binaries in an npm package are "normal" npm bins. This works for free for any npm 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.

joneshf avatar Jan 20 '21 11:01 joneshf

Thanks for opening this @joneshf!

I'm certainly open to discuss the matter, and even merge a patch for this. My only wish in this case is that you'd available to help me out to debug related issues that might arise due to this change: I feel a bit stretched with Spago's maintenance lately, so the last thing I'd like to happen is for my workload to increase due to patches we merge. If that's fine by you then we can move forward with this :slightly_smiling_face:

f-f avatar Jan 20 '21 13:01 f-f

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.

hdgarrood avatar Jan 20 '21 15:01 hdgarrood

My only wish in this case is that you'd available to help me out to debug related issues that might arise due to this change: I feel a bit stretched with Spago's maintenance lately, so the last thing I'd like to happen is for my workload to increase due to patches we merge.

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 target. The package.json could change to something like:

{
  …
  "bin": {
    "spago": "./spago",
    "spago_node": "./spago.js"
  },
  …
}

Where spago.js is basically what spago is now but for all three platforms:

#!/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" npm bin. Everyone else is unaffected. Hopefully that wouldn't change your maintenance burden much. Would that work?

joneshf avatar Jan 23 '21 19:01 joneshf

@f-f Any thoughts on my previous comment?

joneshf avatar Mar 01 '21 15:03 joneshf

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 https://github.com/purescript/spago/issues/564 and https://github.com/yarnpkg/berry/issues/882

hdgarrood avatar Mar 01 '21 19:03 hdgarrood

@joneshf sorry, I somehow missed the notification on this one - thank you for the ping. I have only one question about the change you propose: would defining another bin target mean that users would get a spago_node executable in their PATH?

f-f avatar Mar 01 '21 23:03 f-f

No worries.

It shouldn't show up on the path. It should be like any other bin: it'd be available for npx, yarn run, and similar.

joneshf avatar Mar 08 '21 07:03 joneshf

Oh, I see that the recommended install is with --global: https://github.com/purescript/spago/tree/72315e003857e2a6c6e5904716d440925990ec21#installation. In that case, yes it would show up on the path.

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.

joneshf avatar Mar 08 '21 07:03 joneshf

I'm also fine with any name, as long as it doesn't start with spago - this is so that we don't mess up with shell autocompletion and accidentally confuse users about "which one is the correct one to use"

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?

f-f avatar Mar 08 '21 09:03 f-f

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 bins some time ago. The conclusion I came to was that native bins are not officially supported, but they've been working through emergent behavior of all the tooling involved being forgiving. A quick search to re-find that information has yielded nothing, so I can't really point to something specific–nor validate that my conclusion was correct.

joneshf avatar Mar 08 '21 14:03 joneshf

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?

f-f avatar Mar 08 '21 14:03 f-f

Oh sorry, I think I see.

The only workaround I can think of would be someone maintaining another npm package that provided a "normal" bin. Other than that, there is no real workaround for what's being asked. I say that because the ask isn't to fix one specific problem with one specific tool, it's to allow spago to Just Work™ for any node tooling. Sort of like how spago's current bin works not because npm did something specific to address spago, but because npm provides something the platform can understand based on the bin file (whether that's the actual native spago binary or something generated by cmd-shim).

If there's some configuration or npm package to automatically wrap a native binary in a "normal" bin on Linux/macOS while falling back to the "normal" bin on Windows, that would also be a workaround. I'm not familiar with anything that does that, though.

joneshf avatar Mar 08 '21 16:03 joneshf

The only workaround I can think of would be someone maintaining another npm package that provided a "normal" bin.

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

f-f avatar Apr 07 '21 12:04 f-f

Fair enough. What's the next step here, then?

joneshf avatar Apr 20 '21 14:04 joneshf

@joneshf I'd merge a PR that:

  • copies over the npm directory to another one with the code for the new package
  • updates the CI code to deploy the new package: https://github.com/purescript/spago/blob/41ad739614f4f2c2356ac921308f9475a5a918f4/.github/workflows/release.yml#L112-L117

f-f avatar Apr 20 '21 20:04 f-f

The new Spago has a "normal" NPM package, as it's just JS

f-f avatar Sep 16 '23 10:09 f-f

Hey thanks for following up on this! Sorry for not doing anything about this issue for the past…2+ years. Looks like spago has changed quite a bit in that time. I should get up to speed on what all you all have done.

joneshf avatar Sep 18 '23 01:09 joneshf

Time flies 🙂 I hope you're doing well!

f-f avatar Sep 18 '23 05:09 f-f