Added a feature `shards build` #136

tbrand · 2016-11-15T15:34:12Z

Hi all,

This PR is based on crystal-lang/crystal#3538.
I added a build command for shards.

In shard.yml, we set

targets:
  default:
    main: src/main.cr
    options:
      - -o bin/main
      - --release
  sub:
    main: src/sub.cr

Then,

$ shards build # Build "default" target without specified target
Building: default

$ shards build sub # Build specified target, here it is "sub"
Building: sub

$ shards build all # Build all targets
Building: default
Building: sub

$ shards build fake # Undefined targets raise an error
Target 'fake' is not found

Thanks for your review!

bcardiff · 2016-11-15T16:03:23Z

Related to #110

ysbaddaden

Thanks for taking the time to work on this!

That being said, I would like the feature to be as minimal as possible, and we sadly lacked a proper design beforehand, merely a few random ideas tossed in an issue comment. So, I would like to make many changes, sorry.

the target name is meant to be the name of the resulting binary, which should be installed into the bin directory; also the main must be a complete path (it could be outside the src folder); the command should thus be: ["crystal", "build", target.main, "-o", File.join("bin", target.name)].
let's simplify the command, since default and all won't work as expected (it's the target binary name), so it becomes shards build [options] [target, ...] meaning that:
- shards build will build all configured targets; erroring if none are found (no targets configured);
- shards build one two three will build the one, then two then three targets, one after the other; erroring if a target doesn't exist (missing target foo).
please drop the options Array. I don't think there is much need for it for the time being; options could be passed via the command for example (e.g. shards build --release);
no need to check for the presence of the crystal binary. Let the command fail, and print it with the error message; BTW maybe we'd like to honor an ENV["CRYSTAL"] variable that would default to crystal?
the build command should check the dependencies and install them before trying to build.

Last but not least, thank you for writing integration tests, and taking of your time to work on this feature!

ysbaddaden · 2016-11-15T16:05:13Z

src/spec.cr

+              case pull.read_scalar
+              when "main"
+                target.main = pull.read_scalar
+              when "options"


Let's avoid options for now; they could be passed as options to the command for example:

$ shards build --release

ysbaddaden · 2016-11-15T16:06:30Z

src/commands/command.cr

    getter spec_path : String
    getter lockfile_path : String

    @spec : Spec?
    @locks : Array(Dependency)?

-    def initialize(path)
+    def initialize(path, @sub = nil)


What is sub? Can we avoid it?

ysbaddaden · 2016-11-15T16:08:29Z

src/target.cr

+      @options = [] of String
+    end
+
+    def cmd


What about shell_command? Thought the model shouldn't have to deal with the invocation. Command::Build should be the one generating the command.

ysbaddaden · 2016-11-15T16:09:04Z

src/target.cr

+    property options : Array(String)
+
+    def initialize(@name)
+      super()


Not needed: there are no superclass.

ysbaddaden · 2016-11-15T16:19:36Z

test/integration/build_test.cr

+      Dir.cd(application_path) do
+        File.open "mock.cr", "w"
+        ex = assert_raises(FailedCommand) { run "shards build mock_fake" }
+        assert_match "\e[31mTarget\e[0m 'mock_fake' is not found\n", ex.stdout


Please pass --no-color to the command.

tbrand · 2016-11-16T13:23:22Z

Thanks for your review!
I believe I've implemented what you suggested :)
Please tell me if I haven't do it.

We set

targets:
  test0:
    main: src/main0.cr
  test1:
    main: src/main1.cr

Then

$ shards build # Build main0.cr and main1.cr and generate binaries onto bin/test0 and bin/test1
Building: test0
Building: test1

$ shards build test1 # Build main1.cr and generate a binary onto bin/test1
Building: test1

$ shards build test0 test1 # Same as the top case
Building: test0
Building: test1

One thing that I have to say is that it is quite difficult to know whether an argument is a "target" or "option"
For example,

shards build -aaa bbb

In this case, shards cannot decide bbb is a value of "-aaa" or a target.
If it is okay to decide the order to "shards -> build -> targets -> options", it is possible to regard the input after the arguement with "-" prefix as options.

shards build aaa bbb -ccc ddd
# => aaa and bbb are targets, ddd is a value of -ccc

I can hard code crystal build options, but it is not ideal from the view point of frexibility.

For now, I took first approach.(Regarding the input after the argument with "-" prefix as options)
Please give me comments especially for this implementation.

As a supplement, I don't understand why the test/cli.cr parses a hash manually.
Why don't it use yaml.cr?

ysbaddaden · 2016-11-16T14:44:07Z

Thanks! I'll correctly review this when I have some free time.

bcardiff

After tbrand@58fc34f there are some unused changes i think.

bcardiff · 2016-11-16T15:28:44Z

src/commands/command.cr

@@ -5,6 +5,7 @@ require "../spec"
 module Shards
  abstract class Command
    getter path : String
+    getter sub : String | Nil


bcardiff · 2016-11-16T15:29:36Z

src/commands/command.cr

@@ -24,8 +25,8 @@ module Shards

    abstract def run

-    def self.run(path)
-      new(path).run
+    def self.run(path, sub = nil)


unused change? It should be rolledback probably.

Sija · 2016-11-16T17:47:09Z

src/commands/build.cr

+        parse_args
+        # mkdir bin
+        mkdir_bin
+        if @targets.size == 0


if @targets.empty? :)

Sija · 2016-11-16T17:47:33Z

src/commands/build.cr

+      def parse_args
+        is_option? = false
+        @@args.each do |arg|
+          is_option? = true if arg.starts_with?("-")


You can use Char here: '-'

ysbaddaden · 2016-11-17T14:00:49Z

Squashed and merged as 82fef4d. Thank you!

tbrand · 2016-11-17T14:08:01Z

You meant you keep sub?
82fef4d#diff-3d1bd35545e168a4d913eb6fb9a9f87bR8

taicsuzu added 3 commits November 15, 2016 00:37

parse build command

2d8bed4

added the feature

1cb49b5

added tests

5f56587

ysbaddaden requested changes Nov 15, 2016

View reviewed changes

ysbaddaden added the kind:feature label Nov 15, 2016

ysbaddaden added this to the v0.7.0 milestone Nov 15, 2016

remove options from shard.yml

765d10b

taicsuzu added 2 commits November 16, 2016 23:40

fixed a test

75d2bec

removed unused @sub

58fc34f

bcardiff requested changes Nov 16, 2016

View reviewed changes

Sija reviewed Nov 16, 2016

View reviewed changes

src/commands/build.cr

parse_args

# mkdir bin

mkdir_bin

if @targets.size == 0

Copy link

Contributor

Sija Nov 16, 2016

Choose a reason for hiding this comment

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

if @targets.empty? :)

Sija reviewed Nov 16, 2016

View reviewed changes

removed unused variables

e899213

ysbaddaden self-assigned this Nov 17, 2016

ysbaddaden closed this Nov 17, 2016

RX14 mentioned this pull request Mar 27, 2018

Use as a Build System #110

Open

Sija mentioned this pull request Apr 12, 2018

crystal deps -> shards install crystal-ameba/ameba#42

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a feature `shards build` #136

Added a feature `shards build` #136

tbrand commented Nov 15, 2016 •

edited

Loading

bcardiff commented Nov 15, 2016

ysbaddaden left a comment •

edited

Loading

ysbaddaden Nov 15, 2016

ysbaddaden Nov 15, 2016

ysbaddaden Nov 15, 2016

ysbaddaden Nov 15, 2016

ysbaddaden Nov 15, 2016

tbrand commented Nov 16, 2016

ysbaddaden commented Nov 16, 2016

bcardiff left a comment

bcardiff Nov 16, 2016

bcardiff Nov 16, 2016

Sija Nov 16, 2016

Sija Nov 16, 2016 •

edited

Loading

ysbaddaden commented Nov 17, 2016 •

edited

Loading

tbrand commented Nov 17, 2016

Added a feature shards build #136

Added a feature shards build #136

Conversation

tbrand commented Nov 15, 2016 • edited Loading

bcardiff commented Nov 15, 2016

ysbaddaden left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrand commented Nov 16, 2016

ysbaddaden commented Nov 16, 2016

bcardiff left a comment

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 Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

ysbaddaden commented Nov 17, 2016 • edited Loading

tbrand commented Nov 17, 2016

Added a feature `shards build` #136

Added a feature `shards build` #136

tbrand commented Nov 15, 2016 •

edited

Loading

ysbaddaden left a comment •

edited

Loading

Sija Nov 16, 2016 •

edited

Loading

ysbaddaden commented Nov 17, 2016 •

edited

Loading