[flashrom] [PATCH] Add git support to the Makefile

[Vadim Bendebury]

On Mon, Aug 30, 2010 at 6:48 AM, Carl-Daniel Hailfinger
<c-d.hailfinger.devel.2006 at gmx.net> wrote:
> On 28.08.2010 00:00, Vadim Bendebury wrote:
>> guys, here is the change for your review:
>>
>> http://codereview.chromium.org/3265003/show
>>
>
> I have added my review there. The suggested code breaks for me.
>

Thank you for your comments, I will reply later on today PDT.

>
>> It does not deal with the recent addition which retrieves SVN revision
>> number, that part can be easily added, it mostly makes the git version
>> display much more useful.
>>
>> Ideally we get something like this into the tree.
>>
>
> What exactly was wrong with the patch I posted?
>

mostly the fact that it did not provide time of the commit (or build,
if built in a changed tree), which made it impossible to tell which
version is newer.

>
>> On Fri, Aug 27, 2010 at 2:08 PM, Vadim Bendebury (вб)
>> <vbendeb at google.com> wrote:
>>
>>> On Fri, Aug 27, 2010 at 1:52 PM, David Hendricks <dhendrix at google.com> wrote:
>>>
>>>> [+vadim]
>>>>
>>>> On Fri, Aug 27, 2010 at 1:49 PM, David Hendricks <dhendrix at google.com>
>>>> wrote:
>>>>
>>>>> Looks pretty good so far!
>>>>> For the benefit of everyone who missed the excitement on IRC, there is one
>>>>> major caveat to this patch: Those using a git repository probably have stale
>>>>> SVN metadata, which means the upstream Flashrom SVN information in the
>>>>>
>>>>>
>>> guys, I am joining the party late so to speak, I have a couple of questions:
>>>
>>> - why do you need to integrate shell scripts into the Makefile. It is
>>> perfectly valid to put the code in a file in ./util/
>>>
>>> - when local git tree (or svn tree to that matter) has modified files
>>> - this should be reflected in the version string, to indicate that the
>>> code was built off modified source tree.
>>>
>
> Debatable.
>

well, we might be coming from different angles, I think it is
absolutely  clear in my situation: is a development environment with
multiple copies quickly changing utility flying around, being able to
tell that the two versions are not exactly the same would  save us a
lot of time and frustration.

It is less of a value in the model where the code is on the server and
people just build the utility for themselves without changing it, but
certainly does not hurt there either, does it?


>
>>> - the git hash on its own is not good enough: you can't tell looking
>>> at two hashes which one is more recent, this is why in case svn
>>> information is not available the date should be included.  In case the
>>> source is built off pristine sources, the date should be the git log
>>> date, in case the tree was modified, the date should be the build
>>> date.
>>>
>
> If svn information is not available at all, the report is totally
> worthless anyway.
>

It is of a less of a value, but certainly not worthless: it at least
helps to tell that you are running something different or exactly the
same as somebody else. It is especially helpful when people complaint
about problems with flashrom on their systems.


>
>>> - *anything* isbetter than producing multiple different images with
>>> the same version string
>>>
>
> Of course images with the same version string will differ. Or do you
> really suggest to ship debug builds only, and print the version number
> of every used header file?
>

This is one deep philosophical difference :). Where I come from one
can always be sure that the same version string means the same code,
anything else s not acceptable. In this case it is even easily
achievable, we should have this benefit.

If the git tree is clean, the hash is fixed, there is no version
string variations.  If any file changes, we will have an indication of
this fact (the + appended to the version) and a different build time
each time we build. This could result in different version strings for
the same code, but guarantees that different code will have different
version string. Adding username of the build process would make it
more robust, or we can calculate the hash of the current tree
contents. This sure can and should be discussed further.


>>> I have a patch which achieves most of that, I'll share it with you in
>>> a bit later,
>>>
>
> I think we need a few more iterations to get this through review.
>

I committed it to the chromium.org tree after an LGTM just to be able
to move ahead  other parts of the project, but by no means is it done,

I'll fix the problem you highlighted in the review and will  will
reconcile with your suggested patch, stay tuned,

cheers,
/vb

> Regards,
> Carl-Daniel
>
> --
> http://www.hailfinger.org/
>
>