8000 Add run command by luislavena · Pull Request #546 · crystal-lang/shards · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add run command #546

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 22, 2022
Merged

Add run command #546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
464d03f
Add run command
luislavena Jan 19, 2022
10269a0
Properly handle Windows binaries
luislavena Mar 5, 2022
a6ffabb
Better validation of run integration tests
luislavena Mar 5, 2022
9d63a46
Test ARGV passing to run command
luislavena Mar 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 151 additions & 0 deletions spec/integration/run_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
require "./spec_helper"

private def bin_path(name)
File.join(application_path, "bin", Shards::Helpers.exe(name))
end

describe "run" do
before_each do
Dir.mkdir(File.join(application_path, "src"))
File.write(File.join(application_path, "src", "cli.cr"), "puts __FILE__")
end

after_each do
File.delete File.join(application_path, "shard.yml")
end

it "fails when no targets defined" do
File.write File.join(application_path, "shard.yml"), <<-YAML
name: build
version: 0.1.0
YAML

Dir.cd(application_path) do
ex = expect_raises(FailedCommand) do
run "shards run --no-color"
end
ex.stdout.should contain("Targets not defined in shard.yml")
end
end

it "fails when passing multiple targets" do
File.write File.join(application_path, "shard.yml"), <<-YAML
name: build
version: 0.1.0
targets:
app:
main: src/cli.cr
alt:
main: src/cli.cr
YAML

Dir.cd(application_path) do
ex = expect_raises(FailedCommand) do
run "shards run --no-color app alt"
end
ex.stdout.should contain("Error please specify only one target. If you meant to pass arguments you may use 'shards run target -- args'")
end
end

it "fails when multiple targets, no arg" do
File.write File.join(application_path, "shard.yml"), <<-YAML
name: build
version: 0.1.0
targets:
app:
main: src/cli.cr
alt:
main: src/cli.cr
YAML

Dir.cd(application_path) do
ex = expect_raises(FailedCommand) do
run "shards run --no-color"
end
ex.stdout.should contain("Error please specify the target with 'shards run target'")
end
end

it "runs when only one target" do
File.write File.join(application_path, "shard.yml"), <<-YAML
name: build
version: 0.1.0
targets:
app:
main: src/cli.cr
YAML

Dir.cd(application_path) do
output = run("shards run --no-color")

File.exists?(bin_path("app")).should be_true

output.should contain("Executing: app")
output.chomp.should contain(File.join(application_path, "src", "cli.cr"))
end
end

it "runs specified target" do
File.write File.join(application_path, "shard.yml"), <<-YAML
name: build
version: 0.1.0
targets:
< 10000 /td> app:
main: src/cli.cr
alt:
main: src/cli.cr
YAML

Dir.cd(application_path) do
output = run("shards run --no-color app")

File.exists?(bin_path("app")).should be_true
File.exists?(bin_path("alt")).should be_false

output.should contain("Executing: app")
output.chomp.should contain(File.join(application_path, "src", "cli.cr"))
end
end

it "passes back execution failure from child process" do
File.write File.join(application_path, "src", "fail.cr"), <<-CR
puts "This command fails"
exit 5
CR

File.write File.join(application_path, "shard.yml"), <<-YAML
name: build
version: 0.1.0
targets:
fail:
main: src/fail.cr
YAML

Dir.cd(application_path) do
ex = expect_raises(FailedCommand) do
run "shards run --no-color"
end
ex.stdout.should contain("This command fails")
end
end

it "forwards additional ARGV to child process" do
File.write File.join(application_path, "src", "args.cr"), <<-CR
print "args: ", ARGV.join(',')
CR

File.write File.join(application_path, "shard.yml"), <<-YAML
name: build
version: 0.1.0
targets:
app:
main: src/args.cr
YAML

Dir.cd(application_path) do
output = run("shards run --no-color -- foo bar baz")
output.should contain("Executing: app foo bar baz")
output.should contain("args: foo,bar,baz")
end
end
end
25 changes: 17 additions & 8 deletions src/cli.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module Shards
lock [--update] [<shards>...] - Lock dependencies in `shard.lock` but doesn't install them.
outdated [--pre] - List dependencies that are outdated.
prune - Remove unused dependencies from `lib` folder.
run [<target>] [<options>] - Build and run specified target
update [<shards>...] - Update dependencies and `shard.lock`.
version [<path>] - Print the current version of the shard.

Expand Down Expand Up @@ -56,7 +57,15 @@ module Shards
opts.unknown_args do |args, options|
case args[0]? || DEFAULT_COMMAND
when "build"
build(path, args[1..-1])
targets, build_options = parse_args(args[1..-1])
check_and_install_dependencies(path)

Commands::Build.run(path, targets, build_options)
when "run"
targets, run_options = parse_args(args[1..-1])
check_and_install_dependencies(path)

Commands::Run.run(path, targets, run_options, options)
when "check"
Commands::Check.run(path)
when "init"
Expand Down Expand Up @@ -119,7 +128,7 @@ module Shards
)
end

def self.build(path, args)
def self.parse_args(args)
targets = [] of String
options = [] of String

Expand All @@ -131,13 +140,13 @@ module Shards
end
end

begin
Commands::Check.run(path)
rescue
Commands::Install.run(path)
end
{targets, options}
end

Commands::Build.run(path, targets, options)
def self.check_and_install_dependencies(path)
Commands::Check.run(path)
rescue
Commands::Install.run(path)
end
end

Expand Down
41 changes: 41 additions & 0 deletions src/commands/run.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
require "./command"

module Shards
module Commands
class Run < Command
def run(targets, options, run_options)
if spec.targets.empty?
raise Error.new("Targets not defined in #{SPEC_FILENAME}")
end

# when more than one target was specified
if targets.size > 1
raise Error.new("Error please specify only one target. If you meant to pass arguments you may use 'shards run target -- args'")
end

# when no target was specified
if targets.empty?
if spec.targets.size > 1
raise Error.new("Error please specify the target with 'shards run target'")
else
name = spec.targets.first.name
end
else
name = targets.first
end
Comment on lines +17 to +25
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm too rusty to came with a non-convoluted version of these nested conditions 😬

Better versions are welcome! 😊

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks fine.

But I'm not sure the resulting behaviour is what we should have. At least the discussion in #157 seemed pretty indecisive on that. It might not be that important what we pick, but this alternative is at least not my favourite 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this alternative is at least not my favourite

Can you rephrase or explain this? Not sure I follow. The scenarios I made were:

  1. When no target was indicated and spec contains only one, assume is that target, runs.
  2. When no target was indicated and spec contains more than one, errors out

Looking at the comments in #157 is confusing me:

Yes. No default, unless there is only 1 target.
#157 (comment)

I'm not sure whether the default behaviour is a good idea. shards build defaults to building all. shards run defaults to running one, but only if there is one defined seems off.
#157 (comment)

Always running the first target if no name specified sounds good. That's consistent behaviour.
#157 (comment)

I lean more and more on always specifying the target to run: consistent, no configuration, no documentation, no repeated boring message (that boils down to: you should always specify the target), no surprises (you know what will happen).
#157 (comment)

(skipping the shards isn't a build tool dilema)

I'm ok with making the target mandatory, that will definitely simply the above logic, but I thought of shards run similar to cargo run and the default target selection

Either way, again, we can change it, that is why I sent this PR 😀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we just need to come to a decision in #157. There are contradicting views on that.
Having a PR ready certainly helps to push this forward ;)


if target = spec.targets.find { |t| t.name == name }
Copy link
Contributor
@Sija Sija Mar 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if target = spec.targets.find { |t| t.name == name }
if target = spec.targets.find(&.name.== name)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the original reads better.

Commands::Build.run(path, [target.name], options)

Log.info { "Executing: #{target.name} #{run_options.join(' ')}" }
status = Process.run(File.join(Shards.bin_path, target.name), args: run_options, output: Process::Redirect::Inherit, error: Process::Redirect::Inherit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
status = Process.run(File.join(Shards.bin_path, target.name), args: run_options, output: Process::Redirect::Inherit, error: Process::Redirect::Inherit)
status = Process.run(File.join(Shards.bin_path, target.name), args: run_options, output: :inherit, error: :inherit)

Copy link
Contributor Author
@luislavena luislavena Mar 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about this, however decided to follow the same pattern/style used in build command:

https://github.com/crystal-lang/shards/blob/master/src/commands/build.cr#L46-L48

Could change if really needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way is good

unless status.success?
exit status.exit_code
end
else
raise Error.new("Error target #{name} was not found in #{SPEC_FILENAME}")
end
end
end
end
end
0