Added recipe for libyubikey #929

tudor-nazarie · 2016-12-14T17:58:53Z

Added the recipe for the Yubico YubiKey authentication device library and associated tools.

fbrosson

Good start, thanks!
Minor changes are needed, however, to add support for building for secondary architectures, and to fix a few minor errors.
(I did not test the resulting recipe yet but will do so asap.)

fbrosson · 2016-12-14T22:13:53Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+CHECKSUM_SHA256="04edd0eb09cb665a05d808c58e1985f25bb7c5254d2849f36a0658ffc51c3401"
+REVISION="1"
+LICENSE="BSD (2-clause)"
+COPYRIGHT="Yubico AB"


Could you please change the order of these lines to:

SUMMARY="Low level C SDK for the Yubico YubiKey authentication device" DESCRIPTION="This package make up the low-level C software development kit for the \ Yubico YubiKey authentication device." HOMEPAGE="https://developers.yubico.com/yubico-c/" COPYRIGHT="2008-2015 Yubico AB" LICENSE="BSD (2-clause)" REVISION="1" SOURCE_URI="https://developers.yubico.com/yubico-c/Releases/libyubikey-$portVersion.tar.gz" CHECKSUM_SHA256="04edd0eb09cb665a05d808c58e1985f25bb7c5254d2849f36a0658ffc51c3401"

(Minor changes to COPYRIGHT and re-wrapped DESCRIPTION.)

(See the HaikuPorter Guidelines for more info)

fbrosson · 2016-12-14T22:19:12Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+	cmd:modhex = $portVersion
+	cmd:ykparse = $portVersion
+	cmd:ykgenerate = $portVersion
+	lib:libyubikey


Should be:

PROVIDES=" libyubikey$secondaryArchSuffix = $portVersion cmd:modhex$secondaryArchSuffix = $portVersion cmd:ykparse$secondaryArchSuffix = $portVersion cmd:ykgenerate$secondaryArchSuffix = $portVersion lib:libyubikey$secondaryArchSuffix = 0.1.7 "

fbrosson · 2016-12-14T22:22:53Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+
+PROVIDES_devel="
+	libyubikey_devel = $portVersion
+	devel:libyubikey


Should be:

PROVIDES_devel=" libyubikey${secondaryArchSuffix}_devel = $portVersion devel:libyubikey$secondaryArchSuffix = 0.1.7 " REQUIRES_devel=" libyubikey$secondaryArchSuffix == $portVersion base "

fbrosson · 2016-12-14T22:29:39Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+LICENSE="BSD (2-clause)"
+COPYRIGHT="Yubico AB"
+
+ARCHITECTURES="x86_gcc2 x86"


Should be:

ARCHITECTURES="x86_gcc2 x86 x86_64" SECONDARY_ARCHITECTURES="x86_gcc2 x86"

fbrosson · 2016-12-14T22:38:41Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+
+REQUIRES="
+    haiku >= $haikuVersion
+    "


REQUIRES should go right after PROVIDES, and should be:

REQUIRES=" haiku$secondaryArchSuffix lib:libcurl$secondaryArchSuffix lib:libusb$secondaryArchSuffix "

fbrosson · 2016-12-14T22:40:12Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+    "
+
+BUILD_REQUIRES="
+"


BUILD_REQUIRES=" haiku${secondaryArchSuffix}_devel devel:libcurl$secondaryArchSuffix devel:libusb$secondaryArchSuffix "

fbrosson · 2016-12-14T22:43:02Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+    haiku_devel >= $haikuVersion
+    cmd:gcc
+    cmd:make
+    cmd:awk


Should be:

BUILD_PREREQUIRES=" cmd:awk cmd:gcc$secondaryArchSuffix cmd:make "

fbrosson · 2016-12-14T22:48:07Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+    make install
+
+    prepareInstalledDevelLibs libyubikey
+   	packageEntries devel $developDir


Could you please change this to:

rm $libDir/libyubikey.la prepareInstalledDevelLib libyubikey fixPkgconfig packageEntries devel \ $developDir

(I'm not sure yet for the rm $libDir/libyubikey.la because I don't know if that file is being generated.)

I'm not sure that removing make install is right since it gives me an error about being unable to stat some files.

fbrosson · 2016-12-14T22:50:53Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+BUILD()
+{
+    runConfigure ./configure --disable-dependency-tracking
+    make $jobArgs


Can you please use tabs (instead of white spaces)?

tudor-nazarie · 2016-12-15T15:32:27Z

Hello, thanks for your feedback. I have made the requested changes and updated the commit, bar the removal of make install without which the build fails.

PS. Sorry if I messed something up with GitHub's review system. I'm not entirely sure, but I think I did. I'm still trying to get the hang of it.

fbrosson

Thanks for your update! Your recipe is almost perfect!
You just missed a couple of cosmetic edits which I had not emphasized in my first review.
I'm also adding something I forgot, to remove the man page on secondary arch builds.

fbrosson · 2016-12-15T16:09:14Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+COPYRIGHT="2008-2015 Yubico AB"
+LICENSE="BSD (2-clause)"
+REVISION="1"
+SOURCE_URI="https://developers.yubico.com/yubico-c/Releases/libyubikey-1.13.tar.gz"


Please change SOURCE_URI to:

SOURCE_URI="https://developers.yubico.com/yubico-c/Releases/libyubikey-$portVersion.tar.gz"

(I just replaced "1.13" by "$portVersion")

fbrosson · 2016-12-15T16:53:06Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+	cmd:ykgenerate$secondaryArchSuffix = $portVersion
+	lib:libyubikey$secondaryArchSuffix = 0.1.7
+	"
+


Please remove the blank line between PROVIDES and REQUIRES.

fbrosson · 2016-12-15T16:53:36Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+	libyubikey${secondaryArchSuffix}_devel = $portVersion
+	devel:libyubikey$secondaryArchSuffix = 0.1.7
+	"
+


Please remove the blank line between PROVIDES_devel and REQUIRES_devel.

fbrosson · 2016-12-15T16:56:57Z

sys-auth/libyubikey/libyubikey-1.13.recipe

@@ -0,0 +1,67 @@
+SUMMARY="Low level C SDK for the Yubico YubiKey authentication device"
+DESCRIPTION="This package make up the low-level C software development kit for the Yubico YubiKey authentication device."


Hmm, after all, I don't like the "This package make the" (taken from upstream) because it has a grammar error (missing "s" in "makes"). Shall we drop that part? Here's what I would use:

DESCRIPTION="Low-level C software development kit for the Yubico YubiKey \ authentication device."

(BTW, we need to split the line otherwise it would be > 80 chars.)

fbrosson · 2016-12-15T17:08:49Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+	haiku${secondaryArchSuffix}_devel
+	lib:libcurl$secondaryArchSuffix
+	lib:libusb$secondaryArchSuffix
+"


We need a tab before the closing double-quotes.

fbrosson · 2016-12-15T17:22:31Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+	prepareInstalledDevelLibs libyubikey
+	fixPkgconfig
+	packageEntries devel \
+			$developDir


Can you please add a few extra changes to INSTALL?
This is the layout we like to have:

INSTALL() { make install rm $libDir/libyubikey.la prepareInstalledDevelLib libyubikey fixPkgconfig if [ -n "$secondaryArchSuffix" ]; then rm -rf $manDir fi packageEntries devel \ $developDir }

The main change here is that we remove $manDir if we are building for a secondary arch.

Notice that I replaced prepareInstalledDevelLibs by prepareInstalledDevelLib.
This is not required but it is welcome, since we only have one lib here.

BTW, in my previous review I should have mentioned that I was not willing to drop the "make install". Actually, we cannot control the number of lines GitHub puts in the context, so I should have copied the whole INSTALL ;-)
Hopefully you took the right decision, keeping it ;-)

tudor-nazarie · 2016-12-15T17:57:56Z

Hello again and again thanks for your time. I made the changes you specified and I hope everything is in order this time.

fbrosson

Hello again and again thanks for your time. I made the changes you specified and I hope everything is in order this time.

You are welcome!

I think we're almost done: 2 cosmetic edits remaining.
(I forgot to report an ampty line and you forgot to remove a tab. ;-)

fbrosson · 2016-12-15T18:21:41Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+	lib:libcurl$secondaryArchSuffix
+	lib:libusb$secondaryArchSuffix
+	"
+


Please remove the blank line between BUILD_REQUIRES and BUILD_PREREQUIRES.

So, just to be sure, there should be no blank lines between variable definitions, right? But are they ok between the BUILD, INSTALL and TEST functions?

Edit: now I see that we've separated them in blocks of 2, separated by blank lines. I think I get it now

Sorry, I had missed your message.
And I just realized I forgot to mention our HaikuPorter Guidelines!
https://github.com/haikuports/haikuports/wiki/HaikuPorter-Guidelines#ordering

BTW, if you would like to work on other recipes, we have a lot of these Create a recipe for ... tasks ;-)

fbrosson · 2016-12-15T18:23:17Z

sys-auth/libyubikey/libyubikey-1.13.recipe

+	fi
+
+	packageEntries devel \
+			$developDir


We only need 2 tabs in line 67.

tudor-nazarie · 2016-12-15T18:39:08Z

I updated the commit with the requested cosmetic changes.

fbrosson · 2016-12-15T18:49:37Z

Thanks @xdizzaster for your great work!

fbrosson · 2016-12-15T19:22:16Z

It means it has been merged ;-)
Here it is:
https://github.com/haikuports/haikuports/tree/master/sys-auth/libyubikey
BTW, with this merge, libyubikey is the very first recipe to be added in category sys-auth ;-)

korli · 2016-12-21T07:47:51Z

I've commented on the commit 58be167

fbrosson · 2016-12-21T08:03:27Z

OK , thanks @korli . I'll fix that.
BTW, I'm trying to figure out whether or not we can avoid bumping REVISION since the only change is the addition of two "compag >= 0".
I think we can keep REVISION unchanged without any consequences, but I can bump it if it is safer.
What would you do?

fbrosson · 2016-12-21T08:05:25Z

Oops, I just noticed there were 2 other errors. So I guess we really need to bump REVISION.

fbrosson · 2016-12-21T08:46:13Z

Hmm, libyubikey is currently using libusb-0.1.so from libusb_compat.
While at it, should I make it use libusb-1.0.so from libusb ?
If yes, I'll replace

	lib:libusb$secondaryArchSuffix

by

	lib:libusb_1.0$secondaryArchSuffix

and

	devel:libusb$secondaryArchSuffix

by

	devel:libusb_1.0$secondaryArchSuffix

Thanks!

fbrosson · 2016-12-21T09:12:44Z

Fixed.

tudor-nazarie force-pushed the libyubikey branch from a028307 to b29acbc Compare December 14, 2016 18:05

fbrosson requested changes Dec 14, 2016

View reviewed changes

tudor-nazarie force-pushed the libyubikey branch 5 times, most recently from 7818976 to ca8c311 Compare December 15, 2016 15:29

fbrosson requested changes Dec 15, 2016

View reviewed changes

tudor-nazarie force-pushed the libyubikey branch from ca8c311 to 4d7dccc Compare December 15, 2016 17:55

tudor-nazarie force-pushed the libyubikey branch from 4d7dccc to 8958353 Compare December 15, 2016 18:00

fbrosson requested changes Dec 15, 2016

View reviewed changes

Added recipe for libyubikey

f4b19d1

tudor-nazarie force-pushed the libyubikey branch from 8958353 to f4b19d1 Compare December 15, 2016 18:38

fbrosson approved these changes Dec 15, 2016

View reviewed changes

fbrosson merged commit 58be167 into haikuports:master Dec 15, 2016

tudor-nazarie deleted the libyubikey branch December 15, 2016 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added recipe for libyubikey #929

Added recipe for libyubikey #929

tudor-nazarie commented Dec 14, 2016

fbrosson left a comment

fbrosson Dec 14, 2016

fbrosson Dec 14, 2016

fbrosson Dec 14, 2016

fbrosson Dec 14, 2016

fbrosson Dec 14, 2016

fbrosson Dec 14, 2016

fbrosson Dec 14, 2016

fbrosson Dec 14, 2016

tudor-nazarie Dec 15, 2016

fbrosson Dec 14, 2016

tudor-nazarie commented Dec 15, 2016

fbrosson left a comment

fbrosson Dec 15, 2016

fbrosson Dec 15, 2016

fbrosson Dec 15, 2016

fbrosson Dec 15, 2016

fbrosson Dec 15, 2016

fbrosson Dec 15, 2016

tudor-nazarie commented Dec 15, 2016 •

edited

fbrosson left a comment

fbrosson Dec 15, 2016

tudor-nazarie Dec 15, 2016 •

edited

fbrosson Dec 15, 2016

fbrosson Dec 15, 2016

tudor-nazarie commented Dec 15, 2016

fbrosson commented Dec 15, 2016

fbrosson commented Dec 15, 2016

korli commented Dec 21, 2016

fbrosson commented Dec 21, 2016

fbrosson commented Dec 21, 2016

fbrosson commented Dec 21, 2016

fbrosson commented Dec 21, 2016

		@@ -0,0 +1,67 @@
		SUMMARY="Low level C SDK for the Yubico YubiKey authentication device"
		DESCRIPTION="This package make up the low-level C software development kit for the Yubico YubiKey authentication device."

Added recipe for libyubikey #929

Added recipe for libyubikey #929

Conversation

tudor-nazarie commented Dec 14, 2016

fbrosson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor-nazarie commented Dec 15, 2016

fbrosson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor-nazarie commented Dec 15, 2016 • edited

fbrosson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor-nazarie Dec 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor-nazarie commented Dec 15, 2016

fbrosson commented Dec 15, 2016

fbrosson commented Dec 15, 2016

korli commented Dec 21, 2016

fbrosson commented Dec 21, 2016

fbrosson commented Dec 21, 2016

fbrosson commented Dec 21, 2016

fbrosson commented Dec 21, 2016

tudor-nazarie commented Dec 15, 2016 •

edited

tudor-nazarie Dec 15, 2016 •

edited