|
|
Created:
10 years, 6 months ago by vb Modified:
9 years, 6 months ago Reviewers:
Luigi Semenzato CC:
chromium-os-reviews_chromium.org, Randall Spangler, gauravsh, Bill Richardson, Luigi Semenzato Base URL:
ssh://git@chromiumos-git/vboot_reference.git Visibility:
Public. |
DescriptionKeep 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.
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed review comments. #Patch Set 3 : Do not automatially `git add' the modied file. #Messages
Total messages: 10 (0 generated)
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?
. 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.
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
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.
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
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 >
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 >
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 >> >
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! |