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

Interest in Linux and macOS npm bins being "normal?" #735

Closed
joneshf opened this issue Jan 20, 2021 · 18 comments
Closed

Interest in Linux and macOS npm bins being "normal?" #735

joneshf opened this issue Jan 20, 2021 · 18 comments

Comments

@joneshf
Copy link
Member

joneshf commented Jan 20, 2021

Issue

According to this comment:

spago/npm/spago

Lines 2 to 3 in 960a310

// This file is only used on Windows. In *nix NPM installations it's replaced
// with the actual spago binary.
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.

@f-f
Copy link
Member

f-f commented Jan 20, 2021

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 🙂

@hdgarrood
Copy link
Contributor

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.

@joneshf
Copy link
Member Author

joneshf commented Jan 23, 2021

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

joneshf commented Mar 1, 2021

@f-f Any thoughts on my previous comment?

@hdgarrood
Copy link
Contributor

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

@f-f
Copy link
Member

f-f commented Mar 1, 2021

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

@joneshf
Copy link
Member Author

joneshf commented Mar 8, 2021

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

joneshf commented Mar 8, 2021

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.

@f-f
Copy link
Member

f-f commented Mar 8, 2021

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?

@joneshf
Copy link
Member Author

joneshf commented Mar 8, 2021

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.

@f-f
Copy link
Member

f-f commented Mar 8, 2021

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?

@joneshf
Copy link
Member Author

joneshf commented Mar 8, 2021

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.

@f-f
Copy link
Member

f-f commented Apr 7, 2021

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

@joneshf
Copy link
Member Author

joneshf commented Apr 20, 2021

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

@f-f
Copy link
Member

f-f commented Apr 20, 2021

@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:
    cd npm
    cp ../README.md ./README.md
    cp ../CONTRIBUTING.md ./CONTRIBUTING.md
    cp ../LICENSE ./LICENSE
    npm install
    npm publish --non-interactive --access public

@f-f
Copy link
Member

f-f commented Sep 16, 2023

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

@f-f f-f closed this as completed Sep 16, 2023
@joneshf
Copy link
Member Author

joneshf commented Sep 18, 2023

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.

@f-f
Copy link
Member

f-f commented Sep 18, 2023

Time flies 🙂 I hope you're doing well!

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

No branches or pull requests

3 participants