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.

Sign up for GitHub

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

Jump to bottom

cryfs: trigger bottle rebuild #35958

Closed
wants to merge 2 commits into from
Closed

cryfs: trigger bottle rebuild #35958

wants to merge 2 commits into from

Conversation

smessmer
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • 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 <formula>)?

This fixes #35947

@smessmer smessmer mentioned this pull request Jan 13, 2019
Closed
6 tasks
@freedom1400

This comment has been minimized.

Sign in to view
@fxcoudert
Copy link
Member

Can we add a test that looks at what library this is linked against?

MikeMcQuaid
MikeMcQuaid requested changes Jan 22, 2019
View reviewed changes
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Nice work on the test!

Formula/cryfs.rb Outdated
# wrong. For example there was an ABI incompatibility issue between the crypto++ version
# the cryfs bottle was compiled with and the crypto++ library installed by homebrew to.
Dir.mktmpdir do |basedir|
Dir.mktmpdir do |mountdir|
Copy link
Member

Choose a reason for hiding this comment

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

Could either of these be a plain mkdir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be, but I think mktmpdir is better because it automatically cleans up after the block is exited.

Copy link
Member

Choose a reason for hiding this comment

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

@smessmer Anything in test do's working directory is also automatically cleaned up so these mktmpdir are superfluous and can be replaced with mkdir 👍

Formula/cryfs.rb Outdated
# the cryfs bottle was compiled with and the crypto++ library installed by homebrew to.
Dir.mktmpdir do |basedir|
Dir.mktmpdir do |mountdir|
assert_match "Mounting filesystem", pipe_output("#{bin}/cryfs -f \"#{basedir}\" \"#{mountdir}\"", "password")
Copy link
Member

Choose a reason for hiding this comment

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

  • Can use 2>&1 to avoid printing error messages.
  • Can use single quotes to avoid needing to escape the "

Formula/cryfs.rb Outdated
assert_match "Mounting filesystem", pipe_output("#{bin}/cryfs -f \"#{basedir}\" \"#{mountdir}\"", "password")
end
end
print("Note: A permission error in the previous line is expected and still counts as successful test run\n")
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this.

@SMillerDev SMillerDev added the needs response Needs a response from the issue/PR author label Jan 24, 2019
@smessmer
Copy link
Contributor Author

Thanks. I addressed your comments.

Btw, there was a new upstream release 0.9.10. So instead of bumping the revision number, I switched to the new upstream release.

@SMillerDev
Copy link
Member

@BrewTestBot test this please

@SMillerDev SMillerDev added build failure CI fails while building the software CI-requeued PR has been re-added to the queue and removed needs response Needs a response from the issue/PR author labels Jan 28, 2019
@smessmer
Copy link
Contributor Author

smessmer commented Jan 28, 2019 •
edited

CI fails with a "directory not empty" error message. Not sure how to fix this, maybe actually a bug in homebrew, see Homebrew/brew#4989

@MikeMcQuaid MikeMcQuaid mentioned this pull request Jan 28, 2019
Merged
6 tasks
@MikeMcQuaid MikeMcQuaid reopened this Jan 28, 2019
@MikeMcQuaid MikeMcQuaid reopened this Jan 28, 2019
@smessmer
Copy link
Contributor Author

ok tests seem to have passed now, thanks. Can we merge this?

@MikeMcQuaid
Copy link
Member

@smessmer Can you remove the mktmpdir first? Thanks!

@MikeMcQuaid
Copy link
Member

Installation failing on High Sierra with what looks like a Boost issue: https://jenkins.brew.sh/job/Homebrew%20Core%20Pull%20Requests/37186/version=high_sierra/testReport/junit/brew-test-bot/high_sierra/install_cryfs/

@MikeMcQuaid
Copy link
Member

Reran it and it was fine 🎉.

Thanks so much for your contribution! Without people like you submitting PRs we couldn't run this project. You rock, @smessmer!

@lock lock bot added the outdated PR was locked due to age label Mar 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees
No one assigned
Labels
build failure CI fails while building the software CI-requeued PR has been re-added to the queue outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cryfs bottle built against wrong cryptopp version
5 participants
@smessmer @freedom1400 @fxcoudert @SMillerDev @MikeMcQuaid