ocaml
Print a warning when encountering common shebang mistakes in package build scripts
Desired Behavior
While building packages dune should print a message when a bash script fails to run due to the script using the commonly-used but non-portable shebang: !#/bin/bash
/bin/bash is not guaranteed to be present, and some Linux distros (e.g. NixOS) and various BSDs put their bash in different locations. Packages that run bash scripts during their build with an incorrect shebang are likely to evade detection by CI if the CI server only tests on common distros. Notably this currently affects the package bin_prot.v0.17.0 - a dependency of core.v0.17.0.
Example
When building a dune project that depends on core.v0.17.0, if dune fails to run the cflags.sh script in bin_prot's build command because #!/bin/bash does not exist, dune should detect this case and print a message explaining the situation.
Note that Opam doesn't currently solve this problem. On my NixOS machine I cannot install core with Opam:
#=== ERROR while compiling bin_prot.v0.17.0 ===================================#
# context 2.3.0 | linux/x86_64 | ocaml.5.2.1 | https://opam.ocaml.org#8f63148a9025a7b775a069a6c0b0385c22ad51d3
# path /tmp/tmp.Bgu6gYMNvE/_opam/.opam-switch/build/bin_prot.v0.17.0
# command ~/.opam/opam-init/hooks/sandbox.sh build dune build -p bin_prot -j 31
# exit-code 1
# env-file ~/.opam/log/bin_prot-125876-e03ffb.env
# output-file ~/.opam/log/bin_prot-125876-e03ffb.out
### output ###
# File "xen/dune", lines 1-6, characters 0-111:
# 1 | (rule
# 2 | (targets cflags.sexp)
# 3 | (deps
# 4 | (:first_dep cflags.sh))
# 5 | (action
# 6 | (bash "./%{first_dep} > %{targets}")))
# (cd _build/default/xen && /run/current-system/sw/bin/bash -e -u -o pipefail -c './cflags.sh > cflags.sexp')
# /run/current-system/sw/bin/bash: line 1: ./cflags.sh: cannot execute: required file not found
(Line 1 of cflags.sh is #!/bin/bash).
Note that this issue has been fixed upstream in bin_prot, however a new version hasn't been released. It's an easy mistake to make and I think it would be better if dune could handle it rather than requiring individual packages to be fixed.
I would suggest that it prints a warning. If the shebang is wrong, it should be corrected in some ways and people should be aware that we try to take the "optimistic path".
I would suggest that it prints a warning. If the shebang is wrong, it should be corrected in some ways and people should be aware that we try to take the "optimistic path".
Yeh agreed, it shouldn't just silently work, but print a warning as well so people have the chance to fix the problem upstream.
I somewhat disagree that we should put additional magic into broken packages to make them work. When a user writes stuff, they have a reasonable assumption that the build system will execute stuff as written instead of taking the code just as an inspiration and then doing something else.
We can display a warning if a script is using #!/bin/bash but if a package does the wrong thing it is not Dune's responsibility to save authors from themselves. It is the authors or distributors job to fix it (or declare certain platforms as unsupported).
In the specific case, bin_prot can be patched on the opam-repository side to make it work with Nix and I think a patch is much preferable and standard than some build system magic. Furthermore, the patch will go away over time as a new version is released, wheras shebang rewriting will have to stay forever because it deprioritizes fixing the package issues.
Hi @gridbugs , I hope you're having a great day. Please one question... is this fix for only bash shebangs? (#!/bin/bash), or would it also apply to other interpreters as well — like Python, Perl, etc, if they also have hardcoded paths?
Good point @Leonidas-from-XIV. Ok rather than automatically fixing the problem, let's just print a warning when dune fails to execute a script with a suspect shebang. Best case scenario this helps the user debug their problem and they submit a patch to the opam repo.
@kayodegigz let's just handle bash shebangs for now to keep things simple, as I suspect they will be by far the most common.
(I changed the description of this issue to be about printing a message rather than automatically fixing the problem.)
Hi @gridbugs , I looked through the codebase and I found that the commands are defined in the src/dune_engine/action.ml file. Just wanted to confirm I'm looking in the right direction. Thanks
@kayodegigz it would be nice if we could avoid changing anything inside src/dune_engine as changes inside that module require extra scrutiny and reviews tend to take much longer.
Looking closer at how one might go about solving this issue and I no longer think this is a good first issue. The logic for executing actions lives in src/dune_engine/action_exec.ml, and I think that's where we'd have to add additional logic to check for broken shebangs. Another complication is that the motivation for this issue was building the package bin_prot.v0.17.0, which fails to execute the action (bash "./%{first_dep} > %{targets}"). I was hoping it used the (run ...) action instead of the (bash ...) action as in the former you know exactly which program was being run. The problem with bash is it executes an arbitrary bash command, so the question of which file contains the incorrect shebang is no longer straightforward to answer.
I'm removing the "good first issue" tag and unassigning you. Sorry for the runaround.