Add run command #546

luislavena · 2022-03-05T18:32:13Z

Introduces shards run, similar to shards build, that builds and runs the specified target, providing a unified interface to developers.

With this, a developer can directly use shards run <target> in a single step without having first to compile (build) and then execute it with ./bin/target. No need for a script or wrapper.

This is inspired by crystal build and crystal run user experience (with the difference that crystal run creates a temporary binary).

This is very helpful for automatic rebuild scenarios, where a process is monitoring your code and can rebuild and relaunch your application.

When invoking shards run, it will first build the target (with the supplied options) and then invoke it, passing the extra options:

$ shards run myapp --stats -- --args

Is the same as:

$ shards build myapp --stats

$ ./bin/myapp --args

If the target exits with error, the exit code is passed back.

This builds on top of the great work done by @denolfe in #298 and aims to resolve #157 initial set of requests.

Sending this out for review and possible address the other points left out in the original PR.

Thank you.
❤️ ❤️ ❤️

Introduces `shards run`, similar to `shards build`, that builds and runs the specified target, providing a unified interface to developers. With this, a developer can directly use `shards run <target>` in a single step without having first to compile (build) and then execute it with `./bin/target`. No need for a script or wrapper. This is inspired by `crystal build` and `crystal run` user experience (with the difference that `crystal run` creates a temporary binary). This is very helpful for automatic rebuild scenarios, where a process is monitoring your code and can rebuild and relaunch your application. When invoking `shards run`, it will first build the target (with the supplied options) and then invoke it, passing the extra options: $ shards run myapp --stats -- --args Is the same as: $ shards build myapp --stats $ ./bin/myapp --args If the target exits with error, the exit code is passed back. Resolves crystal-lang#157 Relates crystal-lang#298 Co-authored-by: Elliot DeNolf <65888+denolfe@users.noreply.github.com>

Update helper to correctly build `app.exe` when testing on Windows.

Make sure integration tests for `run` are evaluating the output of the command and not the direct call to the built executable.

luislavena · 2022-03-05T18:54:58Z

src/commands/run.cr

+        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


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

Better versions are welcome! 😊

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 🤷

but this alternative is at least not my favourite

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

When no target was indicated and spec contains only one, assume is that target, runs.

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 😀

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 ;)

Address comments from original PR asking to validate ARGV passing to child process (it was working before, but this spec confirms it).

Sija · 2022-03-05T21:24:09Z

src/commands/run.cr

+          name = targets.first
+        end
+
+        if target = spec.targets.find { |t| t.name == name }


Suggested change

if target = spec.targets.find { |t| t.name == name }

if target = spec.targets.find(&.name.== name)

IMO the original reads better.

Sija · 2022-03-05T21:25:40Z

src/commands/run.cr

+          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)


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)

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.

Either way is good

luislavena · 2022-03-06T10:17:53Z

One note, if approved, I would like to squash all the changes into a single commit.

Thank you.

beta-ziliani

To me this PR does what I expect. If we want to change the semantics of an unspecified target with multiple targets configured, we can see how to proceed later.

Many thanks for your attention to detail @luislavena <3

beta-ziliani · 2022-03-21T14:13:33Z

src/commands/run.cr

+          name = targets.first
+        end
+
+        if target = spec.targets.find { |t| t.name == name }


IMO the original reads better.

beta-ziliani · 2022-03-21T14:14:58Z

src/commands/run.cr

+          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)


Either way is good

luislavena and others added 3 commits March 5, 2022 19:13

Properly handle Windows binaries

10269a0

Update helper to correctly build `app.exe` when testing on Windows.

Better validation of run integration tests

a6ffabb

Make sure integration tests for `run` are evaluating the output of the command and not the direct call to the built executable.

luislavena commented Mar 5, 2022

View reviewed changes

Test ARGV passing to run command

9d63a46

Address comments from original PR asking to validate ARGV passing to child process (it was working before, but this spec confirms it).

Sija reviewed Mar 5, 2022

View reviewed changes

straight-shoota mentioned this pull request Mar 7, 2022

Run command #157

Closed

beta-ziliani approved these changes Mar 21, 2022

View reviewed changes

straight-shoota approved these changes Mar 21, 2022

View reviewed changes

straight-shoota added this to the v0.17.0 milestone Mar 21, 2022

straight-shoota merged commit a5d1c69 into crystal-lang:master Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add run command #546

Add run command #546

luislavena commented Mar 5, 2022 •

edited

Loading

luislavena Mar 5, 2022

straight-shoota Mar 7, 2022

luislavena Mar 7, 2022

straight-shoota Mar 7, 2022

Sija Mar 5, 2022 •

edited

Loading

beta-ziliani Mar 21, 2022

Sija Mar 5, 2022

luislavena Mar 6, 2022 •

edited

Loading

beta-ziliani Mar 21, 2022

luislavena commented Mar 6, 2022

beta-ziliani left a comment

beta-ziliani Mar 21, 2022

beta-ziliani Mar 21, 2022

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

	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)

Add run command #546

Add run command #546

Conversation

luislavena commented Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sija Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luislavena Mar 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luislavena commented Mar 6, 2022

beta-ziliani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luislavena commented Mar 5, 2022 •

edited

Loading

Sija Mar 5, 2022 •

edited

Loading

luislavena Mar 6, 2022 •

edited

Loading