Re: [PATCH] hash: Allow building with the external sha1dc library

From: Takashi Iwai <tiwai@suse.de>
Date: Aug 12, 2017, 06:50 UTC
Message-ID: <s5h1sohxos8.wl-tiwai@suse.de>
In-Reply-To: <xmqqfucxy5u6.fsf@gitster.mtv.corp.google.com>

On Sat, 12 Aug 2017 02:42:25 +0200, Junio C Hamano wrote:

Show 32 quoted lines

> 
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >
> > I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:
> >
> >     $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f
> >     /tmp/local/include/sha1dc/sha1.h
> >     /tmp/local/bin/sha1dcsum
> >     /tmp/local/bin/sha1dcsum_partialcoll
> >     /tmp/local/lib/libsha1detectcoll.a
> >     /tmp/local/lib/libsha1detectcoll.so.1.0.0
> >     /tmp/local/lib/libsha1detectcoll.la
> >
> > So the upstream library expects you (and it's documented in their README) to do:
> >
> >     #include <sha1dc/sha1.h>
> >
> > But your patch is just doing:
> >
> >     #include <sha1.h>
> >
> > At best this seems like a trivial bug and at worst us encoding some
> > Suse-specific packaging convention in git, since other distros would
> > presumably want to package this in /usr/include/sha1dc/sha1.h as
> > upstream suggests. I.e. using the ambiguous sha1.h name is not
> > something upstream's doing by default, it's something you're doing in
> > your package.
> 
> I do not think I saw any updates to this thread.  Should I consider
> the topic ti/external-sha1dc now abandoned?

Sorry for the silence, as I've been too busy for other tasks (there were too many security bugs in the last weeks in many packages...)

Show 6 quoted lines

> As we have finished Git 2.14 cycle, in preparation for the next one,
> the 'next' branch will be rewound and rebuilt early next week.  I do
> not mind tentatively ejecting some topics that needs fix-ups out of
> 'next' to give them a clean restart.  If there will be a reroll that
> addresses the concerns raised during the discussion, please let me
> know.

Feel free to drop my branch, then. I'm going to resubmit the fix in anyway, hopefully in the next week.

One thing I'm not entirely sure is about including the sha1.h as
  #include <sha1dc/sha1.h>

Although the header is installed to sha1dc/sha1.h as default in the upstream package, the intention of this sha1.h is a sort of replacement of md1 (and possibly other) sha1.h. It's a drop-in. So in one side, including <sha1.h> with some include path is the intended behavior, while <sha1dc/sha1.h> would work (likely in a more safer manner) in practice.

I'm inclined to go for <sha1dc/sha1.h> and fix SUSE sha1dc package before that, but I'd like to hear from others, too.

thanks,

Takashi