Install executables #126

ysbaddaden · 2016-09-23T14:06:47Z

WIP to implement #95 as per #95 (comment)

blocked by ~~#123~~ and maybe #124

RX14

I realise this is a WIP PR but I thought I would note some things regardless.

RX14 · 2016-09-23T14:35:21Z

src/validator.cr

+  end
+
+  module Validate
+    macro error(message)


Does this have to be a macro?

RX14 · 2016-09-23T14:36:32Z

src/validator.cr

+      end
+
+      unless version =~ /\A[0-9.]+(-[-_a-z.])?\Z/
+        warning "version should follow semantic versioning"


Mention calver?

RX14 · 2016-09-23T14:37:44Z

test/integration/install_test.cr

+    }
+
+    with_shard(metadata) do
+      puts run("shards install --no-color", capture: true)


ysbaddaden · 2016-09-23T15:17:24Z

@RX14 bad commit, the src/validator.cr file shouldn't have been there!

Sija · 2016-09-23T17:45:51Z

SPEC.md

+A list of executables to be linked by the shared (Array).
+
+The executables can be of any type or language (e.g., shell, binary, ruby), they
+must have the executable bit set (POSIX platforms) and they must exit in the


exit -> exist

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

I'm changing my point of view, again. We should have stubs instead of copying the executables, in order to have some environments variables about the project, namely APP_ROOT, APP_NAME, SHARD_ROOT as well as a tweaked CRYSTAL_PATH. This will allow to have some information about the project, and allow to run/build crystal code on-the-fly using the project's lib folder for dependencies (for example). This should allow what @bcardiff explained in #95 (comment)

This will require a binstubs command to regenerate the stubs, for example after moving a project to another folder on the computer.

ysbaddaden · 2016-12-21T22:26:05Z

I'm now unconvinced by binstubs 😖 I really prefer simply copying executables into the bin folder. Bonus: it's already implemented and can be merged right now.

For @bcardiff scenario: the executable can assume (or require) to be run from the project's top directory (e.g. bin/generator). That would automatically use the lib folder if calling crystal for example. If the executable is actually a crystal source code with a shebang (#! /usr/bin/env crystal) then it may require dependencies or the shard itself as a dependency (require "name/file") or directly (../lib/name/src/file). The app context could be merely Dir.current or File.dirname(Dir.executable_path); then the shard contents can be found under lib/name, etc.

RX14 · 2016-12-22T00:04:29Z

@ysbaddaden What are the disadvantages of binstubs? It doesn't seem like all too much more code.

dmitryrck · 2017-05-02T19:25:16Z

src/resolvers/resolver.cr

@@ -44,7 +44,7 @@ module Shards
      end
    end

-    protected def install_path
+    def install_path


You have to test this method if you are turning it public #justsaying

No. Writing unit tests for details would merely duplicate the integration test.

I don't agree. If you are creating something public you have to guarantee it is correct and the way to do that is automated tests.

But it is up to you.

This is an application, not a library. You may use it as a library, but at your own risk: I don't guarantee the internal API to be stable.

Good to know that you don't guarantee the internal API to be stable for you or for those who one day would contribute.

Like I said: it is up to you.

Just a workaround before crystal-lang/shards#126 becomes live.

RX14 · 2018-02-07T14:32:02Z

src/package.cr

+          Shards.logger.debug "Install bin/#{name}"
+          source = File.join(resolver.install_path, "bin", name)
+          dest = File.join(bin_path, name)
+          FileUtils.cp(source, dest)


Why not symlink?

Because bin should assume to have binaries? Thus, to prepare a package for example, I can cp bin/foo dist/bin/ rather than cp $(realpath bin/foo) dist/bin/ or digging cp lib/foo/bin/foo dist/bin because bin/foo can't be relied upon?

Maybe we could hardlink?

RX14 · 2018-02-07T14:41:08Z

I agree with @ysbaddaden that executables should just assume they're run from the project root. Both crystal and shards assume this. Let's just merge this as-is.

ysbaddaden · 2018-02-08T11:26:18Z

src/commands/update.cr

@@ -14,6 +14,7 @@ module Shards
            Shards.logger.info "Installing #{package.name} (#{package.version})"
            package.install
          end
+          package.install_executables


Hum, executables haven't been rebuilt if the dependency was already installed, why bother reinstalling it? Or maybe a loose version (reinstall if missing?)

ysbaddaden · 2018-02-08T13:37:34Z

Reworked. It now creates a hardlink on POSIX systems, unless the same executable was already installed (i.e. same inode). On Windows it just copies the executable.

straight-shoota · 2018-02-08T13:56:49Z

src/package.cr

+              next if File.stat(destination).ino == File.stat(source).ino
+              File.delete(destination)
+            end
+            File.link(source, destination)


Just because not being executed on windows doesn't mean the file system necessarily supports hard links. There should be a fallback in case File.link does not work.

windows supports hard links though

Sorry, I'm blinded. To me 1. Windows is stuck on FAT32 file systems, and 2. anything else is POSIX and supports hard links (or we're talking embedded systems and they're out-of-scope).

I'll remove the Windows check and assume POSIX until someone with better win32 knowledge ports shards.

POSIX generally supports hard links, but not every supported filesystem does. That can be an issue for example with remote file systems.

I typically use crystal in a linux VM which mounts the windows host's file system. Creating hard links is not possible on such a file system.

There are probably other issues as well but the main point is: it doesn't need to be a hard link, a symlink or copy work as well (allthough with minor drawbacsk). Hard links are not 100% supported everywhere, so there should be an alternative as fallback.

RX14 · 2018-02-08T15:04:49Z

src/package.cr

+            FileUtils.cp(source, destination)
+          {% else %}
+            if File.exists?(destination)
+              next if File.stat(destination).ino == File.stat(source).ino


I'd just take this out, and either:

always replace

always ignore if it exists.

We can't ignore: updating a shard may build a new executable, if shards made the hardlink, we're all set (no need to copy), but if the destination was changed for some reason, then we should delete/link again.

Well inodes are going to be removed from File::Info because they're platform-specific so there's little point in having this check just to remove it later.

Let's just always try hardlinking, and on an exception try copying. It works on every platform and is simple.

Let's just make File::Info have mtime, file? and directory?. Let's drop everything else. Not cross platform, useless or whatever else. I'll use stat(2) directly to get anything done 😭

Well:

File.link("foo", "bar") # => 0 File.link("foo", "bar") # => Error creating link from foo to bar: File exists (Errno)

The only solutions are:

delete/copy all the time (stupid, waste);

check inodes, delete + link if they're different (smart, efficient).

I'm sorry, but I won't remove the inode check. You'll have to add File::Info#inode for systems that support it, so we have a compile time error on windows, for example.

No, the correct solution is to hide the inode and supply a File::Info#same?(File::Info). On unix it will check if device and inode are equal (but those fields are still hidden). On windows it does whatever. I suppose I should implement this.

RX14 · 2018-02-09T12:31:18Z

src/package.cr

            File.link(source, destination)
-          {% end %}
+          rescue Errno : ex
+            if ex.errno == Errno::EPERM


Does it hurt to try regardless of the error? Stuff like EXDEV... (extremely unlikely)

RX14 · 2018-02-09T13:00:46Z

Apart from the above incredibly tiny nitpick i'm 👍 to merge.

plainas · 2018-02-16T16:53:49Z

OK, the discussion has branched quite a bit since 'executables' was mentioned for the first time.

Personally, I think a simple way of installing binaries is of primary importance. Crystal programs, being, by and large, dependency free self contained libraries, are a perfect fit for small command line utilities.

Being able to run crystal init, write some code, shards install and have a command line tool on the system, is of tremendous convenience. How far is this feature from becoming a reality?

Is #124 still blocking this?

RX14 · 2018-03-30T15:45:10Z

Would be nice to get this in for next release. Could help avoid things like rails cli being global in crystal.

Uses (hard) links on POSIX systems to install the executable, taking care to verify whether the destination isn't already the source (i.e. same inode). On Windows we just copy the file. closes #95

Some filesystems don't support hard links which is reported with the EPERM errno, or we try to link across mounted file systems. These cases are edge cases, but let's avoid errors and merely copy the executable.

Sija · 2018-04-12T10:32:34Z

What's the flow of installing executables specified in targets?
Would below code work?

# shard.yml

# ...

targets:
  foo:
    main: src/cli.cr

scripts:
  postinstall: shards build --no-debug

executables:
  - foo

Great to see this merged btw! 🎉

ysbaddaden · 2018-04-12T12:34:21Z

Yes, but transitive dependencies will prevent building your executable. See #124.

ysbaddaden · 2018-04-12T12:39:26Z

BTW: don't use --no-debug. You're only shaving a few kilobytes —we only generate file:line info by default— and make it damn harder to debug exceptions. You should use --release thought.

Uses (hard) links on POSIX systems to install the executable, taking care to verify whether the destination isn't already the source (i.e. same inode). On Windows we just copy the file. Some filesystems don't support hard links which is reported with the EPERM errno, or we try to link across mounted file systems. These cases are edge cases, but we fallback to copy the executable to avoid errors.

ysbaddaden added the work in progress label Sep 23, 2016

ysbaddaden modified the milestone: v0.7.0 Sep 23, 2016

RX14 reviewed Sep 23, 2016

View reviewed changes

ysbaddaden force-pushed the feature/executables branch from 378bc16 to 85f7e9c Compare September 23, 2016 15:17

Sija reviewed Sep 23, 2016

View reviewed changes

ysbaddaden mentioned this pull request Sep 25, 2016

New Crystal search path algorithm #127

Merged

ysbaddaden force-pushed the feature/executables branch 3 times, most recently from d6bb426 to 4adb052 Compare September 27, 2016 20:33

ysbaddaden added kind:feature and removed work in progress labels Sep 27, 2016

ysbaddaden modified the milestones: v0.7.0, v0.8.0 Nov 17, 2016

ysbaddaden force-pushed the feature/executables branch 2 times, most recently from 5300e03 to 9460a40 Compare November 23, 2016 01:00

TechMagister mentioned this pull request Dec 20, 2016

Rails like command line kemalyst/kemalyst#17

Closed

RX14 mentioned this pull request Dec 21, 2016

Usecase: installing a binary into path #95

Closed

samueleaton mentioned this pull request Jan 2, 2017

Integration into another program samueleaton/sentry#6

Open

dmitryrck reviewed May 2, 2017

View reviewed changes

veelenga added a commit to crystal-ameba/ameba that referenced this pull request Nov 16, 2017

Install binary to shard/bin folder

6a28e21

Just a workaround before crystal-lang/shards#126 becomes live.

RX14 approved these changes Feb 7, 2018

View reviewed changes

ysbaddaden commented Feb 8, 2018

View reviewed changes

ysbaddaden force-pushed the feature/executables branch 2 times, most recently from 455aaa1 to 7393ee4 Compare February 8, 2018 13:37

straight-shoota reviewed Feb 8, 2018

View reviewed changes

RX14 reviewed Feb 8, 2018

View reviewed changes

RX14 reviewed Feb 9, 2018

View reviewed changes

ysbaddaden added 2 commits March 31, 2018 15:31

Install executables

c72907c

Uses (hard) links on POSIX systems to install the executable, taking care to verify whether the destination isn't already the source (i.e. same inode). On Windows we just copy the file. closes #95

Fix: copy if link failed with EPERM or EXDEV

8304e2e

Some filesystems don't support hard links which is reported with the EPERM errno, or we try to link across mounted file systems. These cases are edge cases, but let's avoid errors and merely copy the executable.

ysbaddaden force-pushed the feature/executables branch from ee1dd1f to 8304e2e Compare March 31, 2018 13:31

obskyr mentioned this pull request Mar 31, 2018

Add option to specify destination directory #198

Closed

Fix: early return

31b4695

RX14 approved these changes Apr 1, 2018

View reviewed changes

ysbaddaden merged commit 2491f80 into master Apr 12, 2018

ysbaddaden deleted the feature/executables branch April 12, 2018 09:14

This was referenced Apr 13, 2018

Build with --release flag instead of --no-debug crystal-ameba/ameba#47

Closed

Compiling Crystal code in postinstall #124

Closed

Install executables #126

Install executables #126

Conversation

ysbaddaden commented Sep 23, 2016 • edited Loading

RX14 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

ysbaddaden commented Sep 23, 2016

Choose a reason for hiding this comment

ysbaddaden commented Nov 24, 2016 • edited Loading

ysbaddaden commented Dec 21, 2016

RX14 commented Dec 22, 2016

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

ysbaddaden Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

RX14 commented Feb 7, 2018

Choose a reason for hiding this comment

ysbaddaden commented Feb 8, 2018

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

Choose a reason for hiding this comment

RX14 Feb 8, 2018 • 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

RX14 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Feb 9, 2018

plainas commented Feb 16, 2018

RX14 commented Mar 30, 2018

Sija commented Apr 12, 2018

ysbaddaden commented Apr 12, 2018

ysbaddaden commented Apr 12, 2018

ysbaddaden commented Sep 23, 2016 •

edited

Loading

ysbaddaden commented Nov 24, 2016 •

edited

Loading

ysbaddaden Feb 8, 2018 •

edited

Loading

RX14 Feb 8, 2018 •

edited

Loading

RX14 Feb 9, 2018 •

edited

Loading