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

Fix rake tests on OS X #10

Merged
merged 3 commits into from
Apr 2, 2015
Merged

Fix rake tests on OS X #10

merged 3 commits into from
Apr 2, 2015

Conversation

smspillaz
Copy link
Contributor

There are some platform-related differences on OS X which cause bashcov not to work correctly, relating to differences between BASH_SOURCE, fd-passing and find.

Fixes #8

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ea76eaf on smspillaz:fix-8 into 55a8a91 on infertux:master.

_READLINK = find_executable('readlink')
_GREADLINK = find_executable('greadlink')
READLINK = _GREADLINK.nil? ? _READLINK : _GREADLINK
PS4 = %Q{#{PREFIX}$(#{READLINK} -f ${BASH_SOURCE[0]})/${LINENO}: }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I'm not a big fan of including mkmf here - it's supposed to be used in Makefile scripts and the like but not in "normal" Ruby code. If you look here, you can see it defines a lot of top-level stuff and monkey-patches core types like String.
  2. Why would you use greadlink? Isn't readlink available on OS X?
  3. Doesn't bash find the path to readlink if you simply do PS4 = %Q{#{PREFIX}$(readlink -f ${BASH_SOURCE[0]})/${LINENO}: }?
  4. If not, does it work using something like READLINK = IO.popen("whereis -b readlink").read.match(/\Areadlink:\s([^\s]+)/).captures.first? I know it's kinda ugly and I'm sure there is a better way to do that but you get the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the readlink compatibility fiasco is a little annoying :/. Basically, the version of readlink shipped on OS X is not useful at all - the GNU version always prints the absolute path to the file, the BSD version prints the absolute path to the file only if the file was a symlink. It also doesn't have an option to follow symlinks.

Its possible to install the GNU version of coreutils on OS X, which I understand that most developers do, because the BSD equivalents are very lacking. They're all prefixed with a "g". Hence the reason why we'd find greadlinkfirst and then find readlink.

That said, after having read about mkmf and friends, I've decided to remove the readlink solution and replaced it with something that's a little slower than readlink but should work in our case -$(cd $(dirname ${BASH_SOURCE[0]}); pwd)/$(basename ${BASH_SOURCE[0]}). It won't follow symlinks, but it does get you the absolute path.

@infertux
Copy link
Owner

Just wanted to add that this is great work @smspillaz and I don't mean to scare you away with my comments 😝. I'm just trying to do cross-compatibility properly...

@smspillaz
Copy link
Contributor Author

No problem, I haven't really written any ruby code before in any serious
capacity - so all feedback is good feedback.
On 25 Mar 2015 5:19 am, "Cédric Félizard" [email protected] wrote:

Just wanted to add that this is great work @smspillaz
https://github.com/smspillaz and I don't mean to scare you away with my
comments [image: 😝]. I'm just trying to do
cross-compatibility properly...


Reply to this email directly or view it on GitHub
#10 (comment).

BASH_SOURCE isn't always guaranteed to give an absolute path, so
use readlink to ensure that we get the absolute path to the source
file.
Calling it on its own means that file descriptors don't get passed
to it.
find -executable is only available on GNU. find -perm -111 finds
all executable files by their mode (although it won't find
executable files in access control lists on Linux, which
isn't a problem in this case)
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7eede86 on smspillaz:fix-8 into 55a8a91 on infertux:master.

@infertux infertux merged commit 7eede86 into infertux:master Apr 2, 2015
@infertux
Copy link
Owner

infertux commented Apr 2, 2015

Good stuff! 👍

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.

bashcov tests fail on OS X
3 participants