Postinstall on Windows - Githubissues

oprypin commented 3 years ago

Shards usually specify postinstall: make. But on Windows that line means that make.exe must exist. Usually it is not globally available, of course, and even when it is, it's some quirky alternative implementation of make.

What I've been doing is adding a make.cmd file to my Windows-supporting libs and telling people to run that directly. Unfortunately, even though running just "make" in a CMD shell would invoke that make.cmd file, StartProcess (the current underlying implementation of system() in Crystal) doesn't do that (unconditionally expects .exe extension), so anyone installing a shard that needs postinstall: make on POSIX will always get an error on Windows.

It's not even possible to skip the postinstall step, or stop shards from deleting the lib files altogether on such a failure.

Anyway... Even though I've been arguing that system() should not pick any shell on Windows, specifically for Shards postinstall it would probably be useful to wrap the supplied command into cmd /c. Maybe I'm saying this just because it happens to make my make.cmd hack work. But in any case, I think this is strictly an upgrade from the current state.

Let me know if there are any concerns, or I'll send a PR doing that. Or, well, any other ideas are welcome. Seems hard without allowing OS-specific command lines.

bcardiff commented 3 years ago

I'm not sure what make sense because I rarely use postinstall. When I do I use crystal alone instead of make.

What I know I would like is to have a shards [install|update] --skip-postinstall option and a shards postinstall [--dry-run] [shard names] .

Although it does not offer any input in a more portable postinstall support, it does offer a workaround and a transition.

oprypin commented 3 years ago

This is for libraries that have some part implemented in C. I may also not use make at the top level but something is needed to compile code after installation.

straight-shoota commented 3 years ago

@bcardiff These options are great :+1:

But the postinstall hook should nevertheless work on any system. So running it with cmd /c seems like a good idea, not only if you're using make. This should also work with any script file in the repository where script is the posix variant and script.cmd for windows. Only difficulty is that posix scripts usually need to be invoked by an explicitly relative path (./script) but cmd /c ./script fails. Transforming paths to backslash separators on windows should fix this, though: cmd /c .\script works.

oprypin commented 3 years ago

cmd /c ./script fails

Oh, that sucks. Hmmm. And the slash transform doesn't seem like a safe idea, we can't just replace everything.

straight-shoota commented 3 years ago

Replacing only ./ at the start should be fine and safe. cmd /c .\test/test works.

oprypin commented 3 years ago

That's weirdly intriguing

oprypin commented 3 years ago

@straight-shoota Hmm? I just checked and cmd /c .\test/test does not work. Neither does cmd /c "test/test" (with quotes). It has to be cmd /c .\test\test or cmd /c "test\test". So, since even quotes need to be taken into account, to implement substitutions only for the first arg, you need to fully parse out what it means to be the first arg (and we don't have code for that).

straight-shoota commented 3 years ago

Works for me:

C:\Users\Johannes>type test\test.cmd
echo Hello World

C:\Users\Johannes>cmd /c .\test/test

C:\Users\Johannes>echo Hello World
Hello World

C:\Users\Johannes>cmd.exe /h
Microsoft Windows [Version 10.0.19043.844]
(c) 2020 Microsoft Corporation. Alle Rechte vorbehalten.

oprypin commented 3 years ago

@straight-shoota Well this is quite bizarre. Your example actually works as just .\test +the flag /test, and somehow .\test by itself means .\test\test. If the file's name doesn't match the directory's name, the happy accident doesn't happen anymore.

straight-shoota commented 3 years ago

Oh, that's weird. So it won't give us an easy solution.

I actually had a test.cmd in the working directory and that was executed for .\test, not test/test.cmd.

straight-shoota commented 3 years ago

As a general note: There's no reason to expect make to be available on any system. Postinstall hooks depending on make can fail in more cases than Windows. And even if a make interpreter is available, it could speak any Make dialect. Compatibility issues between BSD and GNU make for example are a common pitfall.

drhuffman12 commented 3 years ago

Apparently, the --skip-postinstall option is broken on Linux, Mac, and Windows.

From https://github.com/crystal-ameba/ameba/issues/230#issuecomment-825877957 :

Ubuntu:

Run shards install --ignore-crystal-version --skip-postinstall
shards install --ignore-crystal-version --skip-postinstall
shell: /usr/bin/bash -e {0}
Resolving dependencies
Fetching https://github.com/stumpycr/stumpy_png.git
Fetching https://github.com/stumpycr/stumpy_bmp.git
Fetching https://github.com/crystal-ameba/ameba.git
Fetching https://gitlab.com/arctic-fox/spectator.git
Fetching https://github.com/askn/faker.git
Fetching https://github.com/crystal-community/hardware.git
Fetching https://github.com/stumpycr/stumpy_core.git
Installing stumpy_core (1.9.1)
Installing stumpy_png (5.0.1)
Installing stumpy_bmp (0.2.0)
Installing ameba (0.14.3)
Unhandled exception: Error opening file with mode 'r': '/home/runner/work/crystal_ray_tracer/crystal_ray_tracer/lib/ameba/bin/ameba': No such file or directory (File::NotFoundError)
Postinstall of ameba: make bin && make run_file (skipped)
from /crystal/src/crystal/system/file.cr:7:7 in 'new'
from /crystal/src/file.cr:627:12 in 'install_executables'
from /shards/src/commands/install.cr:78:11 in 'run:ignore_crystal_version'
from /shards/src/cli.cr:100:9 in '->'
from /crystal/src/primitives.cr:255:3 in 'parse'
from /shards/src/cli.cr:145:3 in '__crystal_main'
from /crystal/src/crystal/main.cr:110:5 in 'main'
from src/env/__libc_start_main.c:94:2 in 'libc_start_main_stage2'
Error: Process completed with exit code 1.

MacOS:

Run shards install --ignore-crystal-version --skip-postinstall
Resolving dependencies
Fetching https://github.com/stumpycr/stumpy_png.git
Fetching https://github.com/stumpycr/stumpy_bmp.git
Fetching https://github.com/crystal-ameba/ameba.git
Fetching https://gitlab.com/arctic-fox/spectator.git
Fetching https://github.com/askn/faker.git
Fetching https://github.com/crystal-community/hardware.git
Fetching https://github.com/stumpycr/stumpy_core.git
Installing stumpy_core (1.9.1)
Installing stumpy_png (5.0.1)
Installing stumpy_bmp (0.2.0)
Installing ameba (0.14.3)
Unhandled exception: Error opening file with mode 'r': '/Users/runner/work/crystal_ray_tracer/crystal_ray_tracer/lib/ameba/bin/ameba': No such file or directory (File::NotFoundError)
Postinstall of ameba: make bin && make run_file (skipped)
from raise<File::Error+>:NoReturn
from File::new<String, String, File::Permissions, Nil, Nil>:File
from Shards::Package#install_executables:Nil
from Shards::Commands::Install@Shards::Command::run:ignore_crystal_version<String, Bool>:(Array(Log::Entry) | Channel(Tuple(Log::Entry, Log::Backend+)) | IO+ | Nil)
from ~procProc(Array(String), Array(String), Nil)@src/cli.cr:52
from OptionParser#parse<Array(String)>:Nil
from __crystal_main
from main
Error: Process completed with exit code 1.

Windows:

Run shards install --ignore-crystal-version --skip-postinstall
Resolving dependencies
Fetching https://github.com/stumpycr/stumpy_png.git
Fetching https://github.com/stumpycr/stumpy_bmp.git
Fetching https://github.com/crystal-ameba/ameba.git
Fetching https://gitlab.com/arctic-fox/spectator.git
Fetching https://github.com/askn/faker.git
Fetching https://github.com/crystal-community/hardware.git
Fetching https://github.com/stumpycr/stumpy_core.git
Installing stumpy_core (1.9.1)
Installing stumpy_png (5.0.1)
Installing stumpy_bmp (0.2.0)
Installing ameba (0.14.3)
Postinstall of ameba: make bin && make run_file (skipped)
Unhandled exception: Error opening file with mode 'r': 'D:\\a\\crystal_ray_tracer\\crystal_ray_tracer\\lib\\ameba\\bin\\ameba.exe': No such file or directory (File::NotFoundError)
Error: Process completed with exit code 1.

bcardiff commented 3 years ago

@straight-shoota says: Oh, that looks like a shards bug. --skip-postinstall should probably skip installing executables, too. Ref: https://github.com/crystal-ameba/ameba/issues/230#issuecomment-825883084

I think the Package#install_executable should warn if the source file does not exist. If --skip-postinstall is used it will warn, which would be expected. But there could be other reasons why that file is missing (like author's error) and warning about it should be enough I think.

I think it would be weird to skip the install executable entirely if --skip-postinstall is used.

WDYT?

straight-shoota commented 3 years ago

Let's discuss this in a dedicated issue, #498

oprypin commented 2 years ago

https://github.com/crystal-lang/shards/issues/468#issuecomment-785483155

you need to fully parse out what it means to be the first arg (and we don't have code for that).

Hm, I suppose with https://github.com/crystal-lang/crystal/pull/12278 we do have the code :D Might be worth reconsidering this solution then.

crystal-lang / shards

Postinstall on Windows #468