Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.sh installers do not work with sh #596

Closed
2 tasks done
dbast opened this issue Dec 23, 2022 · 3 comments · Fixed by #599
Closed
2 tasks done

.sh installers do not work with sh #596

dbast opened this issue Dec 23, 2022 · 3 comments · Fixed by #599
Labels
locked [bot] locked due to inactivity type::bug describes erroneous operation, use severity::* to classify the type

Comments

@dbast
Copy link
Member

dbast commented Dec 23, 2022

Checklist

  • I added a descriptive title
  • I searched open reports and couldn't find a duplicate

What happened?

.sh installers created from current constructor:main branch have #!/bin/sh as shebang, but actually fail when called via sh installer.sh or ./installer.sh with

#16 13.80 ./miniconda.sh: 444: [[: not found
#16 14.81 
#16 14.81 CondaFileIOError: '/opt/conda/pkgs/envs/*/env.txt'. [Errno 2] No such file or directory: '/opt/conda/pkgs/envs/*/env.txt'

though, bash installer.sh … shellcheck tells:

In Miniconda3-py310_22.11.1-1-Linux-aarch64.sh line 444:
    if [[ -f "${env_pkgs}channels.txt" ]]; then
       ^-- SC3010 (warning): In POSIX sh, [[ ]] is undefined.

(shellcheck also has lots of more warnings) … we should add some tests that lints the embedded installer code via shellcheck / checks for bashisms.

Conda Info

No response

Conda Config

No response

Conda list

No response

Additional Context

No response

@dbast dbast added the type::bug describes erroneous operation, use severity::* to classify the type label Dec 23, 2022
@jaimergp
Copy link
Contributor

The installation instructions always mentioned bash so I wasn't aware we offering any POSIX guarantees. That said, if we can do the same with POSIX compat without making it super cumbersome, then I'd be ok with such a PR.

+1 on the linters. Not just shell, but also Python. The whole project needs some consistency in style, docstrings and whatnot... I didn't do it during all the previous work to minimise diffs, but once we are done with shipping features, we should start work on this.

@jaimergp
Copy link
Contributor

Oh, and I noticed recently the scripts do not use set -eu so we might be ignoring some errors.

@dbast
Copy link
Member Author

dbast commented Dec 23, 2022

We can also change the shebang to #!/bin/bash, if doing all the logic via POSIX sh compatibility turns out to be too cumbersome.

Agreed to linting and set -eu. Several projects in the conda org have a nice .pre-commit-config.yaml + workflow that can be used without much work. Though the .sh header template has be rendered first before applying some linting (so pre-commit does not help here), thats why a test that renders+lints is maybe easier to handle.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity type::bug describes erroneous operation, use severity::* to classify the type
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants