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

libeatmydata 130 (new formula) #106536

Closed
wants to merge 6 commits into from
Closed

libeatmydata 130 (new formula) #106536

wants to merge 6 commits into from

Conversation

johnsonjh
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

libeatmydata 130 (new formula)

@BrewTestBot BrewTestBot added the new formula PR adds a new formula to Homebrew/homebrew-core label Jul 25, 2022
Formula/libeatmydata.rb Outdated Show resolved Hide resolved
@johnsonjh
Copy link
Contributor Author

johnsonjh commented Jul 26, 2022

@carlocab

It works fine - it actually has no user visible changes - it prevents datasync/fsync calls from working - instead they finish instantly.

For the canonical example:

time mysql-test-run --do-test=innodb
real 3m36.053s, user 0m42.323s, sys 0m2.844s

time eatmydata mysql-test-run --do-test=innodb
real 2m19.610s, user 0m41.939s, sys 0m2.356s

So we save more than a third of the time, but a crash or interruption in power would most likely corrupt the database - the most common use case is accelerating a CI/CD process or a "make test" where the data is ephemeral making the integrity of the data itself on disks unimportant.

The tool doesn't require root at all - it works fine as any user.

The test to verify it is actually working is what requires root on macOS.

On Linux, the simple test is:
image

As you can see, no sync when eatmydata is used.

On MacOS, I'd normally use dtrace to test it's working, as I would on FreeBSD and Solaris and OpenIndiana - macOS only allows DTrace calls as root (I typo's and used strace instead of sync, but that's irrelevant to illustrate the problem):

image

image

So, it works just fine on MacOS - it just can't be easily automatically tested without elevated privileges.

I could write a test in C that depends on some differences between libeatmydata sync() and OS sync() to detect if libeatmydata sync is working, but this would be more brittle and more opaque than what I have now.

Almost all of the shell script test in the formula is error checking to give different exit status levels to report exactly why it failed.

Removing these checks and giving a simple "pass" (0) or "fail" (non-zero) would probably result in a one-liner.

If you know any other system call tracing facility on MacOS that doesn't require elevated privileges, I'll use it - I'm most familiar with Solaris, and luckily MacOS includes dtrace, inherited from FreeBSD.

I checked the upstream tests - they use dtrace's dtruss too.

@carlocab
Copy link
Member

We can just run eatmydata and check it doesn't crash, since we're unable to use dtruss.

Verify that the wrapper doesn't crash; on Linux
also verify that it functions using strace(1).
Formula/libeatmydata.rb Outdated Show resolved Hide resolved
Formula/libeatmydata.rb Outdated Show resolved Hide resolved
Formula/libeatmydata.rb Outdated Show resolved Hide resolved
@BrewTestBot BrewTestBot added automerge-skip `brew pr-automerge` will skip this pull request and removed automerge-skip `brew pr-automerge` will skip this pull request labels Jul 27, 2022
carlocab
carlocab previously approved these changes Jul 27, 2022
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks!

@carlocab carlocab added the ready to merge PR can be merged once CI is green label Jul 27, 2022
@johnsonjh
Copy link
Contributor Author

johnsonjh commented Jul 27, 2022

Thanks!

Making good progress going through my ~/.local/bin and getting what I use all the time that's not already in brew - but should - be added. :) Hopefully the rest will be easy, and sorry to subject you to my Ruby learning experience.

Copy link
Member

@alebcay alebcay left a comment

Choose a reason for hiding this comment

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

Thanks for your recent contributions and activity @johnsonjh! Just a few thoughts here.

Formula/libeatmydata.rb Show resolved Hide resolved
Formula/libeatmydata.rb Outdated Show resolved Hide resolved
@johnsonjh
Copy link
Contributor Author

Thanks for your recent contributions and activity @johnsonjh! Just a few thoughts here.

I noticed another small problem - the oldest version of MacOS supported by Brew doesn't support the readlink -f which is used by the wrapper script, so coreutils actually would be required as a runtime dependency as well - I do have a pure POSIX shell version of this function, but it would be a 40 line patch, so depending on coreutils seems the best option.

I'll report this upstream as well.

Formula/libeatmydata.rb Outdated Show resolved Hide resolved
@johnsonjh
Copy link
Contributor Author

I'll report this upstream as well.

stewartsmith/libeatmydata#28

@johnsonjh
Copy link
Contributor Author

Should be GTG (hopefully)

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

LGTM. @alebcay?

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label Aug 29, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants