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

libfido2 1.3.0 (new formula) #50326

Closed
wants to merge 1 commit into from

Conversation

xanderlent
Copy link
Contributor

@xanderlent xanderlent commented Feb 17, 2020

(This is a revival of PR #46072, which is now used by OpenSSH.)

Co-authored-by: Alexander Lent [email protected]

  • 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 --new-formula <formula> (after doing brew install <formula>)?

There are multiple open issues with this PR, I'll take another look at it later this week:

  • libcbor is not yet upstream (see libcbor 0.5.0 (new formula) #50305 for why)
  • Manual pages aren't installed in the correct location
  • There are probably additional ways to test this library.
  • Yubico's official releases page gives different URLs than the GitHub URL we're using, but the artifacts are identical.
  • libudev is a Linux-only dependency
  • Upstream prefers LibreSSL to OpenSSL. (Possibly true of OpenSSH-portable as well.) We're using OpenSSL for OpenSSH portable, probably best to link dependencies against the same library.

This was referenced Feb 17, 2020
@xanderlent xanderlent force-pushed the libfido2-pr branch 3 times, most recently from 99bc5a0 to 7557676 Compare February 19, 2020 06:01
Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Test suggestion

Formula/libfido2.rb Outdated Show resolved Hide resolved
@fxcoudert fxcoudert added the new formula PR adds a new formula to Homebrew/homebrew-core label Feb 19, 2020
@xanderlent xanderlent force-pushed the libfido2-pr branch 2 times, most recently from 62dd3d2 to 0eb5e65 Compare February 19, 2020 17:18
@xanderlent
Copy link
Contributor Author

xanderlent commented Feb 19, 2020

Questions for reviewers:

  • Do you thing the second test is useful? It tests different functions with empty parameters, but I'm not sure it actually hits crypto codepaths, since the parameters are empty.
  • Does the homebrew DSL support Linux-only dependencies, for ex depends_on "libuv" => :linux?
  • Yubico prefers releases on their servers, though the tarballs are identical. Should we do the same?

I'm re-examining upstream's support for installing manual pages since they don't seem to be correctly installed in the first place. For now, I've got a partially working copy operation. Fixed.

@jonchang
Copy link
Contributor

Do you thing the second test is useful? It tests different functions with empty parameters, but I'm not sure it actually hits crypto codepaths, since the parameters are empty.

If the Makefile provides a make check step we can use it in the install step to exercise the right code paths since this is a crypto library.

Does the homebrew DSL support Linux-only dependencies, for ex depends_on "libuv" => :linux?

uses_from_macos "libuv"

Yubico prefers releases on their servers, though the tarballs are identical. Should we do the same?

If they're identical we might as well use the Github releases since this means brew livecheck will "just work" with them.

@jonchang
Copy link
Contributor

-- Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE) 
-- Checking for one of the modules 'libcbor'
-- Checking for one of the modules 'libcrypto'
CMake Error at /usr/local/Cellar/cmake/3.16.4/share/cmake/Modules/FindPkgConfig.cmake:707 (message):
  None of the required 'libcrypto' found
Call Stack (most recent call first):
  CMakeLists.txt:82 (pkg_search_module)

This probably needs depends_on "pkg-config" => :build?

@gzm55
Copy link

gzm55 commented Feb 20, 2020

-- Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE) 
-- Checking for one of the modules 'libcbor'
-- Checking for one of the modules 'libcrypto'
CMake Error at /usr/local/Cellar/cmake/3.16.4/share/cmake/Modules/FindPkgConfig.cmake:707 (message):
  None of the required 'libcrypto' found
Call Stack (most recent call first):
  CMakeLists.txt:82 (pkg_search_module)

This probably needs depends_on "pkg-config" => :build?

append /usr/local/opt/[email protected]/lib/pkgconfig to PKG_CONFIG_PATH

Formula/libfido2.rb Outdated Show resolved Hide resolved
Formula/libfido2.rb Outdated Show resolved Hide resolved
Formula/libfido2.rb Outdated Show resolved Hide resolved
@xanderlent
Copy link
Contributor Author

xanderlent commented Feb 20, 2020

Do you thing the second test is useful? It tests different functions with empty parameters, but I'm not sure it actually hits crypto codepaths, since the parameters are empty.

If the Makefile provides a make check step we can use it in the install step to exercise the right code paths since this is a crypto library.

After double-checking, it appears the regression tests are automatically run as part of the build, but only on debug builds. I assume that it would be unusual and unnecessarily complicated to build the formula in debug mode first solely to run tests?

Yubico prefers releases on their servers, though the tarballs are identical. Should we do the same?

If they're identical we might as well use the Github releases since this means brew livecheck will "just work" with them.

Thanks, I'll leave it as is.

(This is a revival of PR Homebrew#46072, which is now used by OpenSSH.)

Thanks the the authors of voro++.rb for the solution to moving the
directory containing the manual pages.

Co-authored-by: Alexander Lent <[email protected]>
@xanderlent
Copy link
Contributor Author

xanderlent commented Feb 20, 2020

Does the homebrew DSL support Linux-only dependencies, for ex depends_on "libuv" => :linux?

uses_from_macos "libuv"

Apologies, the dependency is libudev, which I don't think we package in homebrew-core.

Formula in Homebrew/linuxbrew-core have depends_on "systemd" if OS.linux? # for libudev but that seems to belong in linuxbrew for now, if I understand Homebrew/brew#7028 correctly.

@xanderlent xanderlent marked this pull request as ready for review February 20, 2020 18:30
@issyl0
Copy link
Member

issyl0 commented Feb 20, 2020

Formula in Homebrew/linuxbrew-core have depends_on "systemd" if OS.linux? # for libudev but that seems to belong in linuxbrew for now, if I understand Homebrew/brew#7028 correctly.

Correct. This repo is merged into Homebrew/linuxbrew-core a couple of times a day, then fixes for Linux dependencies are made in PRs there.

@xanderlent
Copy link
Contributor Author

xanderlent commented Feb 20, 2020

Great! I'll open a PR there with the necessary changes once this gets merged.

As a note to myself, those are:
depends_on "systemd" if OS.linux? # libfido2 uses libudev on Linux but IOKit on macOS
mv prefix/"man", share/"man" if OS.mac? # This path bug on macOS might be upstream.

Copy link
Contributor

@zbeekman zbeekman left a comment

Choose a reason for hiding this comment

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

LGTM, provided we can't or don't want to enable compile-time tests.

Copy link
Contributor

@zbeekman zbeekman left a comment

Choose a reason for hiding this comment

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

LGTM

There are no tests besides some regression tests that appear to get run at compile time, and fuzzing that only really happens on Linux. I added a suggestion to double check that the regression tests are being run, but they should be ™️ because it looks like the regress target was added to the special ALL CMake target.

Formula/libfido2.rb Show resolved Hide resolved
@xanderlent
Copy link
Contributor Author

@zbeekman If that was it, then we're good to merge.

Thanks to everyone for contributing to the review of this PR! I really appreciate the feedback and look forward to contributing in the future.

@jonchang jonchang closed this in 74a0aca Feb 22, 2020
@jonchang
Copy link
Contributor

Thanks again @xanderlent for contributing to Homebrew! This formula has been merged.

xanderlent added a commit to xanderlent/linuxbrew-core that referenced this pull request Feb 22, 2020
We introduced libfido2 in Homebrew/homebrew-core#50326, but it has different dependencies on Linux.

Additionally, the upstream CMake code uses share/man/ on Linux but man/ on macOS.
@lock lock bot added the outdated PR was locked due to age label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants