Skip to content

Conversation

@Leonidas-from-XIV
Copy link
Collaborator

This PR follows up #13233 and solves remaining issues shellcheck was complaining about.

I've opted to disable SC2016 as it states:

This suggestion is primarily meant to help newbies who assume single and double quotes are basically the same, like in Python and JavaScript. It's not at all meant to discourage experienced users from using single quotes in general. If you are well aware of the difference, please do not hesitate to permanently disable this suggestion

Writing the code "\$SANDBOX" to satisfy SC is less readable and IMHO worse, thus I would actually suggest we globally disable it.

Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
@Leonidas-from-XIV Leonidas-from-XIV force-pushed the solve-shellcheck-problems branch from 3a37f93 to d34e219 Compare January 8, 2026 15:46
@rgrinberg
Copy link
Member

A few more issues pointed out by CI:

File "test/blackbox-tests/test-cases/pkg/dune", lines 18-23, characters 0-136:
18 | (rule
19 |  (alias runtest)
20 |  (enabled_if %{bin-available:shellcheck})
21 |  (deps helpers.sh)
22 |  (action
23 |   (run %{bin:shellcheck} -s bash %{deps})))
(cd _build/default/test/blackbox-tests/test-cases/pkg && /usr/bin/shellcheck -s bash helpers.sh)

In helpers.sh line 153:
add_mock_repo_if_needed() {
^-- SC2120 (warning): add_mock_repo_if_needed references arguments, but none are ever passed.


In helpers.sh line 231:
      cat "${out}" \
          ^------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.


In helpers.sh line 239:
    cat "${out}" \
        ^------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.


In helpers.sh line 249:
  add_mock_repo_if_needed
  ^---------------------^ SC2119 (info): Use add_mock_repo_if_needed "$@" if function's $1 should mean script's $1.


In helpers.sh line 272:
  cat "${default_lock_dir}"/"$1".pkg \
      ^----------------------------^ SC2002 (style): Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

For more information:
  https://www.shellcheck.net/wiki/SC2120 -- add_mock_repo_if_needed reference...
  https://www.shellcheck.net/wiki/SC2119 -- Use add_mock_repo_if_needed "$@" ...
  https://www.shellcheck.net/wiki/SC2002 -- Useless cat. Consider 'cmd < file...
make: *** [Makefile:92: test] Error 1

@Leonidas-from-XIV
Copy link
Collaborator Author

Yes, it's weird, my local shellcheck does not complain about them. I'll try to fix them going through the CI errors.

Signed-off-by: Marek Kubica <marek@tarides.com>
Signed-off-by: Marek Kubica <marek@tarides.com>
@Alizter
Copy link
Collaborator

Alizter commented Jan 9, 2026

@Leonidas-from-XIV Likely your version is older compared to the one in nixpkgs.

@Leonidas-from-XIV
Copy link
Collaborator Author

@Alizter No, I think the failure is due to Shellcheck 0.10 on Ubuntu, which complains on valid things like ${var:-alternative}.

I have 0.11 (latest release) on my system. I tried to investigate what nixpkgs-unstable ships with but the repo structure is too complex to figure out what is going on without investing too much time.

@Alizter
Copy link
Collaborator

Alizter commented Jan 9, 2026

@Alizter No, I think the failure is due to Shellcheck 0.10 on Ubuntu, which complains on valid things like ${var:-alternative}.

I have 0.11 (latest release) on my system. I tried to investigate what nixpkgs-unstable ships with but the repo structure is too complex to figure out what is going on without investing too much time.

https://search.nixos.org/packages?channel=25.11&show=shellcheck&query=shellcheck

Shows 0.11. Looking at the "source" of the derivation there are many configuration options. So it's likely they chose a different set of default config options to how its packaged on Ubuntu.

@rgrinberg
Copy link
Member

How about we just copy one of these sets of configuration options into our repo and point our shellcheck to use it?

@Leonidas-from-XIV Leonidas-from-XIV merged commit f1dc2ca into ocaml:main Jan 9, 2026
30 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the solve-shellcheck-problems branch January 9, 2026 10:59
@Leonidas-from-XIV
Copy link
Collaborator Author

Looking at the "source" of the derivation there are many configuration options.

For me the "Source" link goes to https://github.com/NixOS/nixpkgs/blob/nixos-25.11/pkgs/development/haskell-modules/generic-builder.nix#L887 which doesn't mention shellcheck or any options (same on the "unstable" tab).

But yes, I think it would be nice to have a consistent set of configuration options in our repo for shellcheck to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants