Use POSIX scripts for de-/activation scripts

conda-forge / r-base-feedstock

A conda-smithy repository for r-base.

BSD 3-Clause "New" or "Revised" License

14 stars 47 forks source link

Use POSIX scripts for de-/activation scripts #313

Closed mbargull closed 5 months ago

mbargull commented 5 months ago

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[ x Reset the build number to 0 (if the version changed)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[ ] Ensured the license file is being packaged.

Fixes gh-221

conda-forge-webservices[bot] commented 5 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

isuruf commented 5 months ago

Can't we use

shellcheck:
  enabled: true

in conda-forge.yml?

mbargull commented 5 months ago

Can't we use
shellcheck:
  enabled: true
in conda-forge.yml?

I wasn't aware of that option to be honest. But no, it wouldn't work since that one has, e.g., --shell=bash hard coded: https://github.com/conda-forge/conda-smithy/blob/v3.36.2/conda_smithy/lint_recipe.py#L798 (and that CLI arg would override any shell=sh directive).

conda-forge-webservices[bot] commented 5 months ago

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict. Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

conda-forge-webservices[bot] commented 5 months ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

isuruf commented 5 months ago

But no, it wouldn't work since that one has, e.g., --shell=bash hard coded:

Maybe that should be added as an option. Can you open an issue in conda-smithy?

As for this PR, I'd rather have this test with shellcheck in build-r-base.sh rather than the test section which requires shellcheck only in build_platform and not target_platform.

wolfv commented 5 months ago

Isn't the shellcheck pretty orthogonal to this fix? :)

mbargull commented 5 months ago

Isn't the shellcheck pretty orthogonal to this fix? :)

That's a question on whether one sees regression tests as orthogonal to fixes or not ;). So, IMO, no, since I'd want us to avoid to regress on this.

mbargull commented 5 months ago

Maybe that should be added as an option. Can you open an issue in conda-smithy?

https://github.com/conda-forge/conda-smithy/issues/1965