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

Portability fixes for configure #270

Merged
merged 1 commit into from
Sep 30, 2019

Conversation

msk
Copy link
Contributor

@msk msk commented Sep 30, 2019

The test command, as well as the [ command, is not required to
know the == operator. Only a few implementations like bash and some
versions of ksh support it.

When you run test foo == foo on a platform that does not support the
== operator, the result will be false instead of true. This can lead
to unexpected behavior.

The `test` command, as well as the `[` command, is not required to
know the `==` operator. Only a few implementations like bash and some
versions of ksh support it.

When you run `test foo == foo` on a platform that does not support the
`==` operator, the result will be false instead of true. This can lead
to unexpected behavior.
@eddelbuettel
Copy link
Member

eddelbuettel commented Sep 30, 2019

I have memories of having fought this before, making me unlikely to wanting to return to this. bash it is. I generally change these things only when CRAN asks me too, and as it works even on their Solaris machine the status quo may not change easily.

We also recommend filing an issue ticket prior to making PRs to not waste effort.

So let's turn this then into the issue you never filed: What type of platform would require this?

@msk
Copy link
Contributor Author

msk commented Sep 30, 2019

Thank you for quick feedback.

NetBSD is one of the platforms requiring this change. I am a developer of pkgsrc, the package management system for NetBSD and many other OS's, and had to add this change as a local patch when I packaged RcppArmadillo for pkgsrc.

I understand your concern about potential problems a build script change like this may cause. However, this PR is not adding anything that RcppArmadillo is not already using. RcppArmadillo's configure uses = (mostly autoconf-generated) more often then ==.

❯ grep "if test " configure | grep " = " | wc -l
      33
❯ grep "if test " configure | grep " == " | wc -l
       6

and even configure.ac has one:

❯ grep "if test " configure.ac | grep " = "
if test "${GXX}" = yes; then

So, changing == to = is not only for portability but also for consistency.

@kevinushey
Copy link
Contributor

FWIW s1 = s2 is mandated by POSIX (https://pubs.opengroup.org/onlinepubs/009695399/utilities/test.html), while s1 == s2 is not. So I would agree that using = rather than == is warranted for portability (even if bash remains the default shell as appropriate).

As far as I know, == is a bash-ism that does exactly the same thing as =, so there shouldn't be any risk.

@eddelbuettel
Copy link
Member

I looked more closely (now that I am back from traveling) and the changes to configure.ac are pretty short and sweet and straight (and affect mostly contributed code, hi @kthohr ;-) ) so I think I'll just merge this. (And we didn't really need a change to configure as it gets generated anyway but doesn't hurt...)

@eddelbuettel eddelbuettel merged commit 0fdb3e3 into RcppCore:master Sep 30, 2019
@kthohr
Copy link
Contributor

kthohr commented Oct 1, 2019

#170

Some of those pre-dated my PR ;-)

@eddelbuettel
Copy link
Member

;-) No worries.

eddelbuettel added a commit that referenced this pull request Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants