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

Intend to package highwayhash for Debian #51

Closed
cdluminate opened this issue Apr 19, 2017 · 8 comments
Closed

Intend to package highwayhash for Debian #51

cdluminate opened this issue Apr 19, 2017 · 8 comments

Comments

@cdluminate
Copy link
Contributor

Highwayhash is a tensorflow dependency. I'm working on the packaging, and I think the package can be uploaded soon as long as I import the latest version of highwayhash.

Preliminary packaging is available at a temporary packaging repository [1].

[1] https://github.com/cdluminate/debian-highwayhash

@cdluminate
Copy link
Contributor Author

cdluminate commented Apr 20, 2017

The pre-upload package seems to be in a good shape: http://debomatic-amd64.debian.net/distribution#experimental/highwayhash/0~20170419-g1f4a24f-1/buildlog

@jan-wassenberg
Copy link
Member

Interesting, the link text works but the tilde seems to have some encoding issue in the "href" (when clicked directly).

Looks like the build succeeded, thumbs up! I'm unfamiliar with debian packaging - can you briefly explain the implications and/or any changes you need for that?

@cdluminate
Copy link
Contributor Author

I'll briefly explain how the current preliminary packaging works. And the final package to be uploaded may be different from the current one.

First let's take a look at the patch stack

  1. the patch for adding install target is submitted as an PR, and the patch in PR is newer.

  2. the another patch, somewhat gross, is for building a shared object file (statically-linked files are not always good for distributions), and to remove those machine specific objects (We need portable files in the package. sip_tree_hash.o requires AVX2, such binary files would bring trouble to users whose CPU has no AVX2 feature, hence removed in that patch).

I didn't look into the code carefully, and there must be a better way instead of just dropping that object file.

FYI: packaging under review

@cdluminate
Copy link
Contributor Author

@jan-wassenberg Some questions:

  1. Which architectures are officially supported? Are the amd64 (aka. x86_64), arm64 (aka. aarch64), and ppc64el ?

The package is nearly ready to be uploaded, as said by a reviewer from the Debian community. (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=860804#24 ). However it failed to build on all architectures other than amd64.

According to Makefile [1], it seems that the fix should be exporting HH_AARCH64 for arm64 and HH_POWER for ppc64el when building the package on corresponding architectures.

  1. is 32-bit platforms thoroughly dropped? e.g. i386, armel, armhf. (Although I know that linux distributions are gradually deprecating i{3,4,5,6}86 architecture)

  2. Which kind of ppc64 do you support? ppc64 (big-endian) or ppc64el (little-endian)?

Your answer will help me to make the package better. Thanks in advance. :-)

P.S. Many thanks to the package reviewer Adam Borowski !

[1] https://github.com/google/highwayhash/blob/master/Makefile#L1

@cdluminate
Copy link
Contributor Author

The package was uploaded to Debian experimental. Closing this issue.

@cdluminate
Copy link
Contributor Author

highwayhash fails to build on many architectures, I'll report this issue in detail soon.

@jan-wassenberg
Copy link
Member

@cdluminate , thank you for explaining! Also great discussion in the debian thread, thanks for linking. It makes sense to remove/not distribute sip_tree_hash, that's superceded by HighwayHash anyway.

Which architectures are officially supported? Are the amd64 (aka. x86_64), arm64 (aka. aarch64), and ppc64el ?

Yes. FYI I am quite unfamiliar with the latter two archs, and can only confirm that example and highwayhash_test compile and run on those platforms given the config here.

is 32-bit platforms thoroughly dropped?

We can't build 32-bit anymore without some workarounds, but I am willing to incorporate patches if anyone needs 32-bit. In particular, _mm_cvtsi64_si128 is x64-only. Some of those (especially in Rotate32By) can be replaced with _mm_cvtsi32_si128, or perhaps _mm_cvtsi64x_si128 is available.

Which kind of ppc64 do you support?

After a patch from IBM, sip_hash seems to work fine on BE as well, but the others were only verified on LE. I'd be happy to incorporate further patches but have only tested on LE.

highwayhash fails to build on many architectures, I'll report this issue in detail soon.

Please do. I imagine os_specific raises #error "Port", anything else?

@cdluminate
Copy link
Contributor Author

@jan-wassenberg

All build logs can be found in this page: https://buildd.debian.org/status/package.php?p=highwayhash&suite=experimental (click Build-attempted to view the buildlog, if you feel confused where it is)

A brief overview:

amd64: ok
arm64: ok, just needs a symbols update
ppc64el: ok, just needs a symbols update
ppc64: built but Mismatch at size 2, need the patch from IBM
x32: built but Mismatch at size 1

#error "Port": mips, mips64el, mipsel, armel, armhf, i386, s390x, alpha, hppa, m68k, powerpc, sparc64, kfreebsd-amd64, kfreebsd-i386

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

No branches or pull requests

2 participants