Skip to content

Adds initial support for external commands#14953

Merged
bcardiff merged 7 commits intocrystal-lang:masterfrom
bcardiff:bcardiff/external-commands
Sep 6, 2024
Merged

Adds initial support for external commands#14953
bcardiff merged 7 commits intocrystal-lang:masterfrom
bcardiff:bcardiff/external-commands

Conversation

@bcardiff
Copy link
Member

@bcardiff bcardiff commented Aug 30, 2024

Add support for external commands. Inspired by git and cabal.

When running crystal ext foo bar if ext is not a built-in command, it will look up for crystal-ext executable in the PATH and run that program. The current compiler location is passed as the CRYSTAL env variable, this can be used to run the compiler from the external commands or access additional information present in crystal env.

# crystal-ext.cr
pp! ENV["CRYSTAL"]?
pp! PROGRAM_NAME
pp! ARGV
% crystal build crystal-ext.cr
% PATH=".:$PATH" ./bin/crystal ext foo bar 
ENV["CRYSTAL"]? # => "/Users/bcardiff/Projects/crystal/master/.build/crystal"
PROGRAM_NAME # => "./crystal-ext"
ARGV # => ["foo", "bar"]

This will help to shrink the compiler, but also open the game for external commands.


  • What kind of spec should be added on the CI for this change? A manual one? Because calling Crystal::Command.run as the rest of the compiler specs will end up calling exec.

  • I'm not sure how/if git and cabal list external commands in the help. I would like to offer some discoverability with some description if possible, but that could be deferred for a later PR probably once we are certain we want this.

@Fryguy
Copy link
Contributor

Fryguy commented Aug 30, 2024

git doesn't document it for some reason, but homebrew does - here's their docs: https://docs.brew.sh/External-Commands (https://github.com/Homebrew/brew/blob/master/docs/External-Commands.md)

@bcardiff
Copy link
Member Author

git offers a way to list all supported commands via git help -a. External commands are shown without description, probably because there is no standard for executable metadata. It seems to manually look for all executables that match git- from what I gather browsing its code https://github.com/git/git/blob/master/help.c#L243

Another alternative would be to have a registry in the compiler of known external commands with some minimal description and in case the command is missing we can point the user to install it.

@luislavena
Copy link
Contributor

Nice work @bcardiff !!! 👏

  • I'm not sure how/if git and cabal list external commands in the help. I would like to offer some discoverability with some description if possible, but that could be deferred for a later PR probably once we are certain we want this.

I thought about this when working on shards-ext command support and without doing crazy IPC-plugins, it could be only possible if -ext CLI implements something like --description and the single response is just that.

But as you said, that might be too much and maybe for another PR. As long crystal --help can list the detected external commands (even without description), then user can simply do crystal ext --help to get the information from the external command itself 😉

Amazing work!
❤️ ❤️ ❤️

@straight-shoota
Copy link
Member

straight-shoota commented Sep 2, 2024

Regarding specs, I think we can run the compiler being testd with a command that matches an executable that we put in PATH.
Something like this:

with_tempfile "subcommand" do |path|
  File.write(path, <<-SH
    #!/bin/sh
    echo $@
    SH
  File.chmod(path, 0700)
  process = Process.new(ENV["CRYSTAL_SPEC_COMPILER_BIN"]? || "bin/crystal", ["subcommand", "foo", "bar"], output: :pipe, env: {"PATH" => File.dirname(path)})
  output = process.output.gets_to_end
  status = process.wait
  status.success?.should be_true
  output.should eq "foo bar"
end

@luislavena
Copy link
Contributor

luislavena commented Sep 2, 2024

@bcardiff and @straight-shoota, reminder to wrap the subcommand executable script in a non-win32 setup, or use a Crystal application instead, as we encountered the same thing for shards subcommands: crystal-lang/shards#631 (comment), and code: https://github.com/crystal-lang/shards/blob/master/spec/integration/subcommand_spec.cr#L4-L17

@bcardiff bcardiff force-pushed the bcardiff/external-commands branch from 89b248b to 9e4d2bd Compare September 3, 2024 19:45
@bcardiff
Copy link
Member Author

bcardiff commented Sep 3, 2024

Rebased on master just in case CI failures were due to outdated branch

@bcardiff
Copy link
Member Author

bcardiff commented Sep 4, 2024

@straight-shoota CI is failing because compiler_spec is running without the the binary being compiled. From what I gather (and remember) bin/ci used to do that. But GitHub workflows don't, or I'm reading things incorrectly.

What should be the current way to make this to pass? Should I guard the spec with a compare_versions(Crystal::VERSION, "1.14.0") ? Would that be actively check in some CI workflow before the actual release?

@straight-shoota
Copy link
Member

straight-shoota commented Sep 4, 2024

Hm, yeah compiler_spec contains only unit tests for the compiler and doesn't build a compiler executable.
primitives_spec does require a freshly-built compiler and basically does integration testing. Conceptually this spec would probably better fit there, despite not being a primitive. We might want to rename primitives_spec to integration_spec maybe? Or perhaps better a separate compiler_integration_spec?
It would be great to have more tests for compiler CLI behaviour anyway.

@ysbaddaden
Copy link
Collaborator

Note that compiler_spec does build an internal compiler, and Crystal::Command.run is very slightly tested in spec/compiler/compiler_spec.cr.

@bcardiff
Copy link
Member Author

bcardiff commented Sep 5, 2024

@ysbaddaden but the Command.run to call the external tool will perform a Process.exec which will make the spec to never return. Isn't it?

I think I will need to add an skip if interpreter in the current state to make CI happy.

I don't fully understand why compiler_spec does not run with a recently build compiler (at least in some workflows), and I see that having a separate integration_specs would be a good fit. But we can defer that for later.

@bcardiff
Copy link
Member Author

bcardiff commented Sep 5, 2024

It seems CI is happy enough with that last skip_file 🤞

@straight-shoota straight-shoota added this to the 1.14.0 milestone Sep 5, 2024
@bcardiff bcardiff merged commit cf15fb2 into crystal-lang:master Sep 6, 2024
@bcardiff bcardiff deleted the bcardiff/external-commands branch September 6, 2024 11:59
@zw963
Copy link
Contributor

zw963 commented Oct 10, 2024

Another alternative would be to have a registry in the compiler of known external commands with some minimal description and in case the command is missing we can point the user to install it.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants