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

[bug] Commands joined with && don't run locally installed binaries #1061

Closed
shellscape opened this issue Sep 20, 2023 · 6 comments
Closed

[bug] Commands joined with && don't run locally installed binaries #1061

shellscape opened this issue Sep 20, 2023 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@shellscape
Copy link

Describe the bug

Commands that are joined with && don't appear to look for or run locally installed binaries.

Steps to reproduce

Create a command:

Install typescript locally into a project, make sure it's not installed globally.

  compile:
    command: rm -rf dist && tsc --version

Run the command. Should result in tsc not found

Remove the leading rm -rf dist && and rerun the command, should output something like Version 5.2.2 and succeed.

Expected behavior

This bit me hard today because I had TypeScript v5.0.4 installed globally, while 5.2.2 was installed locally. There is a strict mode build difference between the two versions, and I was not encountering an error while a collaborator was. We couldn't figure out what the difference was. Finally dawned on me to add tsc --version to the command and noticed the difference in version numbers.

Once I removed the global dependency, moon starting throwing tsc not found.

I was expecting Moon to always look to the local dependencies first, and then look to the global deps. That's been the default behavior of npm/yarn/pnpm for some time.

Screenshots

Environment

MacOS

Additional context

@shellscape shellscape added the bug Something isn't working label Sep 20, 2023
@shellscape
Copy link
Author

Side question: Having to use && seems like it jacks up moon's resolution. I can't imagine parsing the commands people come up with is fun. Why not switch to the format that Github Actions uses for executing multiple commands?

        run: |
          git fetch origin
          git branch -f main origin/main

That ends up being translated to "run": "git fetch origin\ngit branch -f main origin/main\n" which should be a lot easier to split on, then each command could be parsed in isolation.

@milesj
Copy link
Collaborator

milesj commented Sep 20, 2023

Yeah, this most likely will not be supported. Tasks were designed to only be 1 command, not multiple, because this entirely breaks the integrated toolchain. The exception to this is system commands, where running multiple/pipes/redirects work fine, just so long as you don't use language specific commands.

The proper way to do this is with 2 tasks:

  clean:
    command: rm -rf dist
    platform: system
  compile:
    command: tsc --version
    platform: node
    deps: [clean]

@shellscape
Copy link
Author

I get that we could use deps for that, but it seems so unnecessary on the surface. clean would never be run in isolation, for instance. It's additional config that puts the burden on the user.

Not trying to bully you into agreeing, just tossing that out there. Hoping maybe there's a use case that's compelling enough for a slight change in the future :)

@milesj
Copy link
Collaborator

milesj commented Sep 20, 2023

It's either use the toolchain and single commands, or ignore the toolchain and do whatever you want. There's no in between from the context of moon. You could ignore the toolchain and use proto directly, but that'll only work for languages, and not packages (tsc, etc).

For example, say we had a command like rm -rf dist/ && tsc --build && node ./doSomething.js && python some-script. While very contrived, it outlines the complexity very well.

  • How many bins are running? 4
  • Which of these are in the toolchain? 1 if node, 2 if python. 2 unknowns. Maybe 0 if not using the toolchain.
  • Well tsc is node though, so 2 for node? Nope, since it's not statically analyzable. We have no idea that this is an npm module. For all we know it could be a Rust or Deno global.
  • So at this point, we'd have to do a bunch of runtime checks to figure out where everything is, and how to run them. This is non-trivial and costly.

Another piece not mentioned, is that multiple commands does not work with inheritance, especially when merging in additional arguments, or overriding the binary name.

@shellscape
Copy link
Author

OK all of that makes sense. I think we broached a tangential topic a while back as well, because some of this reasoning looks familiar. Would it be worth adding a summary like above to the docs?

@milesj
Copy link
Collaborator

milesj commented Sep 25, 2023

Updated docs, will close.

@milesj milesj closed this as completed Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants