Add more POSIX shell compatibility.

conda-forge / openmpi-feedstock

A conda-smithy repository for openmpi.

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

9 stars 22 forks source link

Add more POSIX shell compatibility. #163

Closed ktlim closed 3 weeks ago

ktlim commented 3 weeks ago

${!} indirection is also a bash-only feature. Also set OMPI_FCFLAGS once.

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[ ] 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.

conda-forge-webservices[bot] commented 3 weeks 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.

ktlim commented 3 weeks ago

@conda-forge-admin, please rerender

minrk commented 3 weeks ago

Thanks for the PR! let's please solve this in #159 instead of separately. Instead of merging this, I'll fix it in #159 to avoid introducing more conflicts.

Can I ask how you encountered this? I've been testing with at least zsh, bash, and sh, all of which don't have an issue. conda-build specifies that build scripts are run in bash, so the if $CONDA_BUILD part should the the only one processed by all shells. But maybe some shells do processing of the whole file differently.

ktlim commented 3 weeks ago

We only encountered the problem with the first line (which was why that was the only fix at first) trying to activate openmpi on Debian under /bin/sh (which is not bash). I thought I had looked around for documentation on conda-build's shell but now I see that it is indeed specified to be bash, so this PR wasn't really necessary.

beckermr commented 3 weeks ago

I guess it depends on how much of the file the shell interprets. If it goes line by line, then we could have not-supported syntax tucked in an if statement and it'd be fine. IDK how the shell actually does things.

minrk commented 3 weeks ago

@ktlim thanks! That's my understanding, and thanks for the fix in #162. We can come back with more fixes if someone encounters a problem, but at least for now I think we can say that this is sh-compatible, and I've tested that it works with /bin/sh for me, at least.