search

dbuenzli / topkg

The transitory OCaml software packager
http://erratique.ch/software/topkg
ISC License
69 stars 25 forks source link

use `command -v` instead of `type in Topkg_os.Cmd.exists #127

Closed hannesm closed 5 years ago

hannesm commented 6 years ago

on OpenBSD, type is not a builtin, but an alias to whence -v - thus ocaml pkg/pkg.ml build --pinned true - which discovers git or hg - fails

similar code has been fixed in bos (see https://github.com/dbuenzli/bos/issues/52) and earlier discussion in #39.

With this change, workarounds (such as https://github.com/mirage/mirage-solo5/pull/30/files#diff-1f947de1e211dae385d88d792a4fd397 and similar https://github.com/hannesm/jackline/blob/afee826c5d297f5c2dfe95dc41b8a80c54be9ef9/opam#L10-L13 are no longer needed).

dbuenzli commented 6 years ago

What you propose here, is not the way it has been fixed in bos.

dbuenzli commented 6 years ago

(I perfectly remember this being the posix way of doing this but I also somehow remember less there was a reason why I didn't use this)

hannesm commented 6 years ago

@dbuenzli yes, this is minimal change which works on the platforms I have access to (FreeBSD, OpenBSD, Linux). I couldn't find a reason in the referenced issues why the command -v route was not further investigated.

dbuenzli commented 6 years ago

I couldn't find a reason in the referenced issues why the command -v route was not further investigated.

The reason is simple, bos avoid any dependency on the platform's default shell (and nowadays simply reimplements the lookup procedure).

hannesm commented 6 years ago

@dbuenzli ic, thanks. why was the topkg code left as is (using type)? would you prefer to copy the "simple reimplementation of the lookup procedure" into topkg?

dbuenzli commented 6 years ago

@dbuenzli ic, thanks. why was the topkg code left as is (using type)?

Because so far it never proved to be a problem.

would you prefer to copy the "simple reimplementation of the lookup procedure" into topkg?

No, I'm willing to go with your proposal once I'm sure it doesn't poses any problem.

hannesm commented 6 years ago

@dbuenzli No, I'm willing to go with your proposal once I'm sure it doesn't poses any problem.

is there something I can do in that regard? FWIW, I first saw this issue in November 2016 (where a friend reported that the build instructions don't work on OpenBSD), but only last weekend managed to get my hands on an OpenBSD machine to find the root cause.

dbuenzli commented 6 years ago

is there something I can do in that regard?

Yes, could you please lookup how this is done in opam (they might depend on unix though) ?

Also cmdliner uses this aswell for paging.

One thing I don't understand is that Sys.command is being used and therefore system(3) so why doesn't the alias get expanded by the call on OpenBSD ? Finally are you sure this is a stock OpenBSD and not a personal tweak ?

hannesm commented 6 years ago

dear @dbuenzli, sorry for my delayed reply.

opam uses "/bin/sh", ["-c"; Printf.sprintf "command -v %s" name] here.

as test case, I use in your test/test.ml the following snippet:

let () =
  Topkg_log.set_level (Some Topkg_log.Debug) ;
  (match Topkg_os.Cmd.exists (Topkg_cmd.v "git") with
   | Error (`Msg e) -> Printf.printf "error with cmd.exists %s\n" e
   | Ok b -> Printf.printf "ok with cmd.exists %b\n" b)

The output on a stock OpenBSD 6.3 (released April 15th) is:

test.native: [EXEC] ['type' 'git' 1>'/dev/null' 2>'/dev/null']
ok with cmd.exists false

a snippet from ktrace:

 30266 test.native CALL  execve(0x19b618e21a2d,0x7f7ffffcbb00,0x7f7ffffcbd58)
 30266 test.native NAMI  "/bin/sh"
 30266 test.native ARGS  
        [0] = "sh"
        [1] = "-c"
        [2] = "'type' 'git' 1>'/dev/null' 2>'/dev/null'"
 30266 sh       RET   execve 0
  8979 test.native RET   vfork 30266/0x763a
...
 30266 sh       NAMI  "/dev/null"
 30266 sh       RET   open 3
 30266 sh       CALL  fcntl(1,F_DUPFD_CLOEXEC,0xa)
 30266 sh       RET   fcntl 10/0xa
 30266 sh       CALL  dup2(3,1)
 30266 sh       RET   dup2 1
 30266 sh       CALL  close(3)
 30266 sh       RET   close 0
 30266 sh       CALL  open(0x1c1a158ab330,0x601<O_WRONLY|O_CREAT|O_TRUNC>,0666<S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH>)
 30266 sh       NAMI  "/dev/null"
 30266 sh       RET   open 3
 30266 sh       CALL  fcntl(2,F_DUPFD_CLOEXEC,0xa)
 30266 sh       RET   fcntl 11/0xb
 30266 sh       CALL  dup2(3,2)
 30266 sh       RET   dup2 2
 30266 sh       CALL  close(3)
 30266 sh       RET   close 0
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/home/hannes/.opam/4.06.1/bin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/home/hannes/bin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/bin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/sbin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/usr/bin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/usr/sbin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/usr/X11R6/bin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/usr/local/bin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/usr/local/sbin/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "/usr/games/type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  stat(0x1c1a659b2110,0x7f7fffff1350)
 30266 sh       NAMI  "./type"
 30266 sh       RET   stat -1 errno 2 No such file or directory
 30266 sh       CALL  write(2,0x1c199ddafc10,0x14)
 30266 sh       GIO   fd 2 wrote 20 bytes
       "sh: type: not found
       "

with the commit in this PR applied, the output:

test.native: [EXEC] ['command' '-v' 'git' 1>'/dev/null' 2>'/dev/null']
ok with cmd.exists true

ktrace

 54588 test.native NAMI  "/bin/sh"
 54588 test.native ARGS  
        [0] = "sh"
        [1] = "-c"
        [2] = "'command' '-v' 'git' 1>'/dev/null' 2>'/dev/null'"
 54588 sh       RET   execve 0
 73779 test.native RET   vfork 54588/0xd53c
...
 54588 sh       NAMI  "/dev/null"
 54588 sh       RET   open 3
 54588 sh       CALL  fcntl(1,F_DUPFD_CLOEXEC,0xa)
 54588 sh       RET   fcntl 10/0xa
 54588 sh       CALL  dup2(3,1)
 54588 sh       RET   dup2 1
 54588 sh       CALL  close(3)
 54588 sh       RET   close 0
 54588 sh       CALL  open(0xf5472d040d0,0x601<O_WRONLY|O_CREAT|O_TRUNC>,0666<S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH>)
 54588 sh       NAMI  "/dev/null"
 54588 sh       RET   open 3
 54588 sh       CALL  fcntl(2,F_DUPFD_CLOEXEC,0xa)
 54588 sh       RET   fcntl 11/0xb
 54588 sh       CALL  dup2(3,2)
 54588 sh       RET   dup2 2
 54588 sh       CALL  close(3)
 54588 sh       RET   close 0
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/home/hannes/.opam/4.06.1/bin/git"
 54588 sh       RET   stat -1 errno 2 No such file or directory
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/home/hannes/bin/git"
 54588 sh       RET   stat -1 errno 2 No such file or directory
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/bin/git"
 54588 sh       RET   stat -1 errno 2 No such file or directory
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/sbin/git"
 54588 sh       RET   stat -1 errno 2 No such file or directory
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/usr/bin/git"
 54588 sh       RET   stat -1 errno 2 No such file or directory
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/usr/sbin/git"
 54588 sh       RET   stat -1 errno 2 No such file or directory
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/usr/X11R6/bin/git"
 54588 sh       RET   stat -1 errno 2 No such file or directory
 54588 sh       CALL  stat(0xf53e6bd8c10,0x7f7ffffd6370)
 54588 sh       NAMI  "/usr/local/bin/git"
 54588 sh       STRU  struct stat { dev=1031, ino=78002, mode=-rwxr-xr-x , nlink=119, uid=0<"root">, gid=7<"bin">, rdev=313320, atime=1525770776<"May  8 11:12:56 2018">, mtime=1525770776<"May  8 11:12:56 2018">, ctime=1526066633<"May 11 21:23:53 2018">.902522708, size=2625171, blocks=5184, blksize=16384, flags=0x0, gen=0x0 }

thanks to my friend benno, who is an OpenBSD developer, the behaviour of their ksh got modified in https://github.com/openbsd/src/commit/b4fe65e7b2796eea01f1acf106e9f7e484264ddf -- unclear how other ksh implementations work.

dbuenzli commented 6 years ago

These ktraces are beautiful but they don't really answer that question:

One thing I don't understand is that Sys.command is being used and therefore system(3) so why doesn't the alias get expanded by the call on OpenBSD ?

In any case I'll likely merge this once I get some time to work on topkg again.

dbuenzli commented 5 years ago

Thanks your patch is in as e57ae7a0be8795ff022a1601daaf11d