Keep temp files in the output directory.

Keep temp files in the output directory.

Also, make sure that the regenerated version gets
automatically added to the set of changed files.

Tested by rebuilding the tree and observing the
`git status' output.

[Luigi Semenzato, 2010-06-24 22:33:52]

On 2010/06/24 21:00:43, vb wrote:
> 

Hey, thank you for fixing this.  Certainly those temp files belong to the build
directory.  

Something may be wrong, though: the hash didn't make it into version.c.  Also, I
am wondering what will happen when the buildbot tries to do the "git add".

The whole thing is a little imperfect.  Maybe the emerge should pass a flag to
the Makefile to prevent recreating version.c?

[Luigi Semenzato, 2010-06-24 22:34:39]

.

http://codereview.chromium.org/2809040/diff/1/2
File firmware/Makefile (right):

http://codereview.chromium.org/2809040/diff/1/2#newcode58
firmware/Makefile:58: git add version.c))
Nit: can you align these two lines?

Also---there may be a problem here when we run this in the buildbot.  Is git add
possibly going to fail?

http://codereview.chromium.org/2809040/diff/1/3
File firmware/version.c (right):

http://codereview.chromium.org/2809040/diff/1/3#newcode1
firmware/version.c:1: char* VbootVersion = "VBOOv=";
Mmm, this isn't right.  One of the files didn't get created?  We should try to
make the build fail, so we can catch this.

[gauravsh, 2010-06-24 22:37:10]

On Thu, Jun 24, 2010 at 3:34 PM,  <semenzato@chromium.org> wrote:
>
> http://codereview.chromium.org/2809040/diff/1/2
> File firmware/Makefile (right):
>
> http://codereview.chromium.org/2809040/diff/1/2#newcode58
> firmware/Makefile:58: git add version.c))
> Nit: can you align these two lines?
>
> Also---there may be a problem here when we run this in the buildbot.  Is
> git add possibly going to fail?

This seems wrong to me. The Makefile should be independent and unaware
of whatever underlying SCM we are using. "git add" should be something
that must be done by the developer and not the Makefile. Just my 2
cents.

>
> http://codereview.chromium.org/2809040/diff/1/3
> File firmware/version.c (right):
>
> http://codereview.chromium.org/2809040/diff/1/3#newcode1
> firmware/version.c:1: char* VbootVersion = "VBOOv=";
> Mmm, this isn't right.  One of the files didn't get created?  We should
> try to make the build fail, so we can catch this.
>
> http://codereview.chromium.org/2809040/show
>



-- 
-g

[vb, 2010-06-24 23:35:50]

http://codereview.chromium.org/2809040/diff/1/2
File firmware/Makefile (right):

http://codereview.chromium.org/2809040/diff/1/2#newcode58
firmware/Makefile:58: git add version.c))
On 2010/06/24 22:34:40, Luigi Semenzato wrote:
> Nit: can you align these two lines?
> 
> Also---there may be a problem here when we run this in the  buildbot.  Is git
add
> possibly going to fail?

This is a good concern. technically speaking buildbot is
never supposed to get to this git statement, because the
version supposed to match the source tree, and it is an
error if it does not.

Come to think of it, maybe we should not store this file in
git at all, and generate it each time on the fly, place it
in the output directory and compile it from there?

http://codereview.chromium.org/2809040/diff/1/3
File firmware/version.c (right):

http://codereview.chromium.org/2809040/diff/1/3#newcode1
firmware/version.c:1: char* VbootVersion = "VBOOv=";
On 2010/06/24 22:34:40, Luigi Semenzato wrote:
> Mmm, this isn't right.  One of the files didn't get created?  We should try to
> make the build fail, so we can catch this.

Gee, I should have looked it it before sending the diff.
There was one $ sign too many in the makefile, fixed.

As for failing the build if version is not generated properly - we should do it,
but we should first decide if we 
want to keep version.c in git.

[vb, 2010-06-24 23:45:14]

On Thu, Jun 24, 2010 at 3:36 PM, Gaurav Shah <gauravsh@chromium.org> wrote:
>
> On Thu, Jun 24, 2010 at 3:34 PM,  <semenzato@chromium.org> wrote:
> >
> > http://codereview.chromium.org/2809040/diff/1/2
> > File firmware/Makefile (right):
> >
> > http://codereview.chromium.org/2809040/diff/1/2#newcode58
> > firmware/Makefile:58: git add version.c))
> > Nit: can you align these two lines?
> >
> > Also---there may be a problem here when we run this in the buildbot.  Is
> > git add possibly going to fail?
>
> This seems wrong to me. The Makefile should be independent and unaware
> of whatever underlying SCM we are using. "git add" should be something
> that must be done by the developer and not the Makefile. Just my 2
> cents.
>

This is a valid concern. But this all business of make modifying a
checked in file seems a bit weird to me.

In defense of my suggestion - this would not alter the state of the
repository, it will just explicitly add the modified file to the list
of files to be committed. Note that the user will have to do something
about it anyway.

On the buiddot this should not be a problem if committed version.c is
right, otherwise it might fail, but it should anyways, because this
would indicate an inconsistent version.c.

But as I said in the previous email, maybe the right thing to do is to
keep this file out of the repo, keep generating it on the fly when
running make. What do you guys think?

cheers,
/v



> >
> > http://codereview.chromium.org/2809040/diff/1/3
> > File firmware/version.c (right):
> >
> > http://codereview.chromium.org/2809040/diff/1/3#newcode1
> > firmware/version.c:1: char* VbootVersion = "VBOOv=";
> > Mmm, this isn't right.  One of the files didn't get created?  We should
> > try to make the build fail, so we can catch this.
> >
> > http://codereview.chromium.org/2809040/show
> >
>
>
>
> --
> -g

[Luigi Semenzato, 2010-06-24 23:45:48]

There are many ways to deal with this and none is perfect.

The goal is to be able to get a tarball from git.chromium.org and
compile it in a way that we can identify where it comes from by
examining the binary (in this case, "strings <binary> | grep VBOO".

The right solution would be to have a git checkout hook that emulates
$Id$ from RCS, which git doesn't have.  Unfortunately git doesn't have
checkout hooks either (that I know).  We could add some code to the
build scripts. They are a mysterious black box which we prefer not to
touch (but be my guest).

So the version identifier has to be checked in.


On Thu, Jun 24, 2010 at 4:35 PM,  <vbendeb@chromium.org> wrote:
>
> http://codereview.chromium.org/2809040/diff/1/2
> File firmware/Makefile (right):
>
> http://codereview.chromium.org/2809040/diff/1/2#newcode58
> firmware/Makefile:58: git add version.c))
> On 2010/06/24 22:34:40, Luigi Semenzato wrote:
>>
>> Nit: can you align these two lines?
>
>> Also---there may be a problem here when we run this in the  buildbot.
>
> Is git add
>>
>> possibly going to fail?
>
> This is a good concern. technically speaking buildbot is
> never supposed to get to this git statement, because the
> version supposed to match the source tree, and it is an
> error if it does not.
>
> Come to think of it, maybe we should not store this file in
> git at all, and generate it each time on the fly, place it
> in the output directory and compile it from there?
>
> http://codereview.chromium.org/2809040/diff/1/3
> File firmware/version.c (right):
>
> http://codereview.chromium.org/2809040/diff/1/3#newcode1
> firmware/version.c:1: char* VbootVersion = "VBOOv=";
> On 2010/06/24 22:34:40, Luigi Semenzato wrote:
>>
>> Mmm, this isn't right.  One of the files didn't get created?  We
>
> should try to
>>
>> make the build fail, so we can catch this.
>
> Gee, I should have looked it it before sending the diff.
> There was one $ sign too many in the makefile, fixed.
>
> As for failing the build if version is not generated properly - we
> should do it, but we should first decide if we
> want to keep version.c in git.
>
> http://codereview.chromium.org/2809040/show
>

[Luigi Semenzato, 2010-06-24 23:53:29]

On Thu, Jun 24, 2010 at 4:44 PM, Vadim Bendebury <vbendeb@chromium.org> wrote:
> On Thu, Jun 24, 2010 at 3:36 PM, Gaurav Shah <gauravsh@chromium.org> wrote:
>>
>> On Thu, Jun 24, 2010 at 3:34 PM,  <semenzato@chromium.org> wrote:
>> >
>> > http://codereview.chromium.org/2809040/diff/1/2
>> > File firmware/Makefile (right):
>> >
>> > http://codereview.chromium.org/2809040/diff/1/2#newcode58
>> > firmware/Makefile:58: git add version.c))
>> > Nit: can you align these two lines?
>> >
>> > Also---there may be a problem here when we run this in the buildbot.  Is
>> > git add possibly going to fail?
>>
>> This seems wrong to me. The Makefile should be independent and unaware
>> of whatever underlying SCM we are using. "git add" should be something
>> that must be done by the developer and not the Makefile. Just my 2
>> cents.
>>
>
> This is a valid concern. But this all business of make modifying a
> checked in file seems a bit weird to me.
>
> In defense of my suggestion - this would not alter the state of the
> repository, it will just explicitly add the modified file to the list
> of files to be committed. Note that the user will have to do something
> about it anyway.
>
> On the buiddot this should not be a problem if committed version.c is
> right, otherwise it might fail, but it should anyways, because this
> would indicate an inconsistent version.c.
>
> But as I said in the previous email, maybe the right thing to do is to
> keep this file out of the repo, keep generating it on the fly when
> running make. What do you guys think?

The firmware is compiled on some old and crufty Windows installation.
We would need to modify the Windows build to generate version.c.  It
would take at least cygwin, probably md5sum.  It may be doable.

Note that there is a similar problem in tpm_lite, which is also part
of the firmware.  There, another file that we generate and then commit
is structures.c.  That one would be practically impossible to generate
on Windows, as it requires the Trousers package.

I don't like this either.  I went through a number of alternatives and
liked them less.  I don't think I exhausted the space, but close.

> cheers,
> /v
>
>
>
>> >
>> > http://codereview.chromium.org/2809040/diff/1/3
>> > File firmware/version.c (right):
>> >
>> > http://codereview.chromium.org/2809040/diff/1/3#newcode1
>> > firmware/version.c:1: char* VbootVersion = "VBOOv=";
>> > Mmm, this isn't right.  One of the files didn't get created?  We should
>> > try to make the build fail, so we can catch this.
>> >
>> > http://codereview.chromium.org/2809040/show
>> >
>>
>>
>>
>> --
>> -g
>

[vb, 2010-06-24 23:59:10]

OK, let's  postpone this discussion (and at the very least take it out
of context of this CL).

I also removed the 'git add' part,

please have another look,

cheers,
/v


On Thu, Jun 24, 2010 at 4:53 PM, Luigi Semenzato <semenzato@chromium.org> wrote:
> On Thu, Jun 24, 2010 at 4:44 PM, Vadim Bendebury <vbendeb@chromium.org> wrote:
>> On Thu, Jun 24, 2010 at 3:36 PM, Gaurav Shah <gauravsh@chromium.org> wrote:
>>>
>>> On Thu, Jun 24, 2010 at 3:34 PM,  <semenzato@chromium.org> wrote:
>>> >
>>> > http://codereview.chromium.org/2809040/diff/1/2
>>> > File firmware/Makefile (right):
>>> >
>>> > http://codereview.chromium.org/2809040/diff/1/2#newcode58
>>> > firmware/Makefile:58: git add version.c))
>>> > Nit: can you align these two lines?
>>> >
>>> > Also---there may be a problem here when we run this in the buildbot.  Is
>>> > git add possibly going to fail?
>>>
>>> This seems wrong to me. The Makefile should be independent and unaware
>>> of whatever underlying SCM we are using. "git add" should be something
>>> that must be done by the developer and not the Makefile. Just my 2
>>> cents.
>>>
>>
>> This is a valid concern. But this all business of make modifying a
>> checked in file seems a bit weird to me.
>>
>> In defense of my suggestion - this would not alter the state of the
>> repository, it will just explicitly add the modified file to the list
>> of files to be committed. Note that the user will have to do something
>> about it anyway.
>>
>> On the buiddot this should not be a problem if committed version.c is
>> right, otherwise it might fail, but it should anyways, because this
>> would indicate an inconsistent version.c.
>>
>> But as I said in the previous email, maybe the right thing to do is to
>> keep this file out of the repo, keep generating it on the fly when
>> running make. What do you guys think?
>
> The firmware is compiled on some old and crufty Windows installation.
> We would need to modify the Windows build to generate version.c.  It
> would take at least cygwin, probably md5sum.  It may be doable.
>
> Note that there is a similar problem in tpm_lite, which is also part
> of the firmware.  There, another file that we generate and then commit
> is structures.c.  That one would be practically impossible to generate
> on Windows, as it requires the Trousers package.
>
> I don't like this either.  I went through a number of alternatives and
> liked them less.  I don't think I exhausted the space, but close.
>
>> cheers,
>> /v
>>
>>
>>
>>> >
>>> > http://codereview.chromium.org/2809040/diff/1/3
>>> > File firmware/version.c (right):
>>> >
>>> > http://codereview.chromium.org/2809040/diff/1/3#newcode1
>>> > firmware/version.c:1: char* VbootVersion = "VBOOv=";
>>> > Mmm, this isn't right.  One of the files didn't get created?  We should
>>> > try to make the build fail, so we can catch this.
>>> >
>>> > http://codereview.chromium.org/2809040/show
>>> >
>>>
>>>
>>>
>>> --
>>> -g
>>
>

[Luigi Semenzato, 2010-06-25 00:11:37]

LGTM, except I noticed a serious bug of mine: the find pattern for "Makefile*"
is going to include the emacs checkpoints (Makefile~).  Can you please change
that to "Makefile" in this CL?

Thanks!