conda-smithy 3.23.0 #246
conda-smithy 3.23.0 #246
Conversation
|
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 ( |
Refreshes the `requirements/host` based on `pyproject.toml`. https://github.com/conda-forge/conda-smithy/blob/a5a45bbdc0fae354345abb85404050f61a16a306/pyproject.toml#L2-L6
Support for `shellcheck` in `conda-smithy` was added a while ago. However the dependency wasn't included here. This adds it. conda-forge/conda-smithy@84652a0
recipe/meta.yaml
Outdated
| @@ -40,6 +43,7 @@ requirements: | |||
| - vsts-python-api | |||
| - toolz | |||
| - scrypt | |||
| - shellcheck | |||
There was a problem hiding this comment.
Support for shellcheck was added in PR ( conda-forge/conda-smithy#1122 ) some time ago, but the requirement was not included here. Have added it. Though please let me know if I'm missing something
There was a problem hiding this comment.
It was removed in https://github.com/conda-forge/conda-smithy-feedstock/pull/188/files because of some issue I don't remember. Maybe shellcheck crashed?
There was a problem hiding this comment.
Hmm...ok. Thanks for that detail. Unfortunate we don't have more history in that PR. Maybe @beckermr remembers more details?
There was a problem hiding this comment.
I don’t recall either way but we don’t currently use it in conda forge for sure.
There was a problem hiding this comment.
Was it related to issue ( conda-forge/conda-smithy#1316 )? AFAICT everything there has been fixed ( conda-forge/conda-smithy#1316 (comment) )
There was a problem hiding this comment.
Thought shellcheck would only be used by the linter (when enabled), which is Linux x86_64 IIUC. If true, why would Linux ppc64le builds matter?
There was a problem hiding this comment.
We wouldn’t be able to even use smithy on ppc64le regardless of the linter.
There was a problem hiding this comment.
We could add shellcheck to run_constrained
Edit: Would that be a fair compromise in general here? Considering we are concerned about fallout from making it a hard requirement?
There was a problem hiding this comment.
Side note: Created PR ( conda-forge/shellcheck-feedstock#13 ) to add ppc64le to shellcheck. Unclear on why there wasn't a bot PR for this. Documented some history about what happened regarding shellcheck builds on other architectures.
There was a problem hiding this comment.
Getting shellcheck to work on ppc64le appears to be non-trivial. Went ahead and moved to run_constrained and added a comment linking to issue ( #248 ). This at least provides a little context to recipe reviewers about what is going on there
| - setuptools >=45 | ||
| - setuptools-scm >=7 | ||
| - tomli >=1.0.0 |
There was a problem hiding this comment.
Added these to line up with upstream build requirements included in this release
| run_constrained: | ||
| # For more details about `shellcheck`, please see this issue. | ||
| # xref: https://github.com/conda-forge/conda-smithy-feedstock/issues/248 | ||
| - shellcheck |
There was a problem hiding this comment.
run_constrained without a version here is a no-op right?
There was a problem hiding this comment.
Yeah it is more documentation than anything
There was a problem hiding this comment.
In that case, would a regular commented out dependency in the run section fulfil the same purpose? Or do you want it in the repodata, explicitly?
There was a problem hiding this comment.
There was some support for doing this above ( #246 (comment) ). So had made this change
No strong feelings about other such changes
There was a problem hiding this comment.
Do we want to make a change here before merging or are we happy to merge?
There was a problem hiding this comment.
Cool, let's merge then. If we decide to handle this another way, we can do that in a new PR
|
Thanks Isuru! 🙏 |
Merge only after success.
This pull request was auto-generated by rever
Closes #247