[PATCH]romfs: change the mechanism that builds the romtool utility

[Patrick Georgi]

Hi,

right now, we still have one victim to the romfs change in the build 
test, the first board in the list. Concurrent build, absolute paths and 
make's inability to combine these two unusual traits of a makefile come 
into play.
The following patch fixes that, and creates a romtool per-build (even 
though that should not be necessary).
Whitespace might be off, I can do a commit tomorrow, if I get an ack.

Signed-off-by: Patrick Georgi

Index: util/newconfig/config.g
===================================================================
--- util/newconfig/config.g     (Revision 4059)
+++ util/newconfig/config.g     (Arbeitskopie)
@@ -2225,13 +2225,15 @@
         writemakefileheader(file, makefilepath)

# main rule
-       file.write("\nall: romtool")
+       file.write("\nall: ")
         for i in buildroms:
                 file.write(" %sfs" % i.name)
         file.write("\n\n")

# romtool rules
-       file.write("\nromtool:\n\tcd $(TOP)/util/romtool; make\n")
+       file.write("\nromtool:\n\t$(MAKE) -C $(TOP)/util/romtool clean 
all\n\tmkdir -p tools\n")
+       file.write("\tcp $(TOP)/util/romtool/tools/rom-mkpayload 
$(TOP)/util/romtool/tools/rom-mkstage tools\n\tcp 
$(TOP)/util/romtool/romtool romtool\n")
+       file.write("\t$(MAKE) -C $(TOP)/util/romtool clean\n")

file.write("include Makefile.settings\n\n")
         for i, o in romimages.items():
@@ -2268,15 +2270,15 @@

romsize = getoption("ROM_SIZE", image)
         # i.name? That can not be right, can it?
-       file.write("%sfs: %s $(TOP)/util/romtool/romtool\n" 
%(i.name,i.name));
+       file.write("%sfs: %s romtool\n" %(i.name,i.name));
         file.write("\trm -f coreboot.romfs\n");
-       file.write("\t$(TOP)/util/romtool/romtool %sfs create %s %s 
%s.bootblock\n" % (i.name, romsize, bootblocksize, i.name))
+       file.write("\t./romtool %sfs create %s %s %s.bootblock\n" % 
(i.name, romsize, bootblocksize, i.name))
         for i in buildroms:
                 for j in i.roms:
                         #failover is a hack that will go away soon.
                         if (j != "failover") and (rommapping[j] != 
"/dev/null"):
-                               file.write("\tif [ -f %s/romfs-support 
]; then $(TOP)/util/romtool/romtool %sfs add-payload %s %s/payload `cat 
%s/romfs-support`; fi\n" % (j, i.name, rommapping[j], j, j))
-               file.write("\t $(TOP)/util/romtool/romtool %sfs print\n" 
% i.name)
+                               file.write("\tif [ -f %s/romfs-support 
]; then ./romtool %sfs add-payload %s %s/payload `cat %s/romfs-support`; 
fi\n" % (j, i.name, rommapping[j], j, j))
+               file.write("\t ./romtool %sfs print\n" % i.name)

file.write(".PHONY: all clean romtool")
         for i in romimages.keys():

[PATCH]romfs: change the mechanism that builds the romtool utility

[Peter Stuge]

Patrick Georgi wrote:
> romtool per-build

Is it too much for us to ask that the cbfs tool is built and
installed into the build system? I think that would be ok.

//Peter

[ron minnich]

On Fri, Apr 3, 2009 at 4:06 PM, Peter Stuge  wrote:
> Patrick Georgi wrote:
> > romtool per-build
>
> Is it too much for us to ask that the cbfs tool is built and
> installed into the build system? I think that would be ok.

that's a reasonable idea.

Just tell people that the tool needs to be executable and in PATH

ron

[PATCH]romfs: change the mechanism that builds the romtool utility

[Carl-Daniel Hailfinger]

On 04.04.2009 01:08, ron minnich wrote:
> On Fri, Apr 3, 2009 at 4:06 PM, Peter Stuge  wrote:
> > Patrick Georgi wrote:
> > > romtool per-build
> >
> > Is it too much for us to ask that the cbfs tool is built and
> > installed into the build system? I think that would be ok.
>
> that's a reasonable idea.
>
> Just tell people that the tool needs to be executable and in PATH

Please don't. That will make incompatible changes for romtool
impossible. Just think of the child^Wpain we had with multiple lzma
implementations. v3 avoids the lzma problem by building lzma in the
build dir so we always know the lzma version.

Regards,
Carl-Daniel

[ron minnich]

On Fri, Apr 3, 2009 at 4:19 PM, Carl-Daniel Hailfinger
 wrote:
> Please don't. That will make incompatible changes for romtool
> impossible. Just think of the child^Wpain we had with multiple lzma
> implementations. v3 avoids the lzma problem by building lzma in the
> build dir so we always know the lzma version.

actually, I installed lar in my path on v3 and only had trouble with
that setup once. In fact, the long term vision for tools like LAR and
romtool is that they be usable and functional outside the build tree.

But this is not a burning issue for me either way.

ron

[PATCH]romfs: change the mechanism that builds the romtool utility

[Patrick Georgi]

Am 04.04.2009 01:06, schrieb Peter Stuge:
> Patrick Georgi wrote:
> > romtool per-build
>
> Is it too much for us to ask that the cbfs tool is built and
> installed into the build system? I think that would be ok.

"into the build system"?

With this patch, it's built in util/romtools and then "installed" into 
the coreboot-build/*/ directories.

Patrick

[PATCH]romfs: change the mechanism that builds the romtool utility

[Peter Stuge]

Patrick Georgi wrote:
> > Is it too much for us to ask that the cbfs tool is built and
> > installed into the build system? I think that would be ok.
>
> "into the build system"?

"on the build system" then.
> With this patch, it's built in util/romtools and then "installed"
> into the coreboot-build/*/ directories.

I meant build the tool, then install to /usr/local/bin
I like that for romcc as well.

If that fails, I think it's ok to at least build each of them in
their util/ directory and run them there.

It would be trivial if we did not insist on build directories, but I
don't believe they will go away.

At any rate, isn't it actually very easy to build all neccessary
tools in a common place and then depend on these in each board?

That way they would only be built once.

//Peter

[PATCH]romfs: change the mechanism that builds the romtool utility

[Patrick Georgi]

That whole discussion is so unrelated to the actual issue the patch 
attempts to solve, it isn't funny.

To reduce the impact of the patch, here's an updated version that 
doesn't change the behaviour in util/romtool at all.
The only thing it fixes is the concurrency issue that currently kills 
the first target in the list of the autobuilder.

Feel free to continue to debate the merit of default install locations 
etc (btw: Solaris wants SVr4 style, ie /opt/coreboot/bin or so. 
/usr/local/bin is not part of their file system standard. Please take 
that into account, if you really want to go down that route), I don't 
think romfs should be considered stable (in terms of interfaces and file 
formats) enough for installation yet, so I'll stay away from that.

Again, the patch is probably not whitespace clean (copy&pasted), but I 
will commit as soon as I get an ack.

Signed-off-by: Patrick Georgi

Index: util/newconfig/config.g
===================================================================
--- util/newconfig/config.g     (Revision 4060)
+++ util/newconfig/config.g     (Arbeitskopie)
@@ -2225,13 +2225,14 @@
         writemakefileheader(file, makefilepath)

# main rule
-       file.write("\nall: romtool")
+       file.write("\nall: ")
         for i in buildroms:
                 file.write(" %sfs" % i.name)
         file.write("\n\n")

# romtool rules
-       file.write("\nromtool:\n\tcd $(TOP)/util/romtool; make\n")
+       file.write("\nromtool:\n\t$(MAKE) -C 
$(TOP)/util/romtool\n\tmkdir -p tools\n")
+       file.write("\tcp $(TOP)/util/romtool/tools/rom-mkpayload 
$(TOP)/util/romtool/tools/rom-mkstage tools\n\tcp 
$(TOP)/util/romtool/romtool romtool\n")

file.write("include Makefile.settings\n\n")
         for i, o in romimages.items():
@@ -2268,15 +2269,15 @@

romsize = getoption("ROM_SIZE", image)
         # i.name? That can not be right, can it?
-       file.write("%sfs: %s $(TOP)/util/romtool/romtool\n" 
%(i.name,i.name));
+       file.write("%sfs: %s romtool\n" %(i.name,i.name));
         file.write("\trm -f coreboot.romfs\n");
-       file.write("\t$(TOP)/util/romtool/romtool %sfs create %s %s 
%s.bootblock\n" % (i.name, romsize, bootblocksize, i.name))
+       file.write("\t./romtool %sfs create %s %s %s.bootblock\n" % 
(i.name, romsize, bootblocksize, i.name))
         for i in buildroms:
                 for j in i.roms:
                         #failover is a hack that will go away soon.
                         if (j != "failover") and (rommapping[j] != 
"/dev/null"):
-                               file.write("\tif [ -f %s/romfs-support 
]; then $(TOP)/util/romtool/romtool %sfs add-payload %s %s/payload `cat 
%s/romfs-support`; fi\n" % (j, i.name, rommapping[j], j, j))
-               file.write("\t $(TOP)/util/romtool/romtool %sfs print\n" 
% i.name)
+                               file.write("\tif [ -f %s/romfs-support 
]; then ./romtool %sfs add-payload %s %s/payload `cat %s/romfs-support`; 
fi\n" % (j, i.name, rommapping[j], j, j))
+               file.write("\t ./romtool %sfs print\n" % i.name)

file.write(".PHONY: all clean romtool")
         for i in romimages.keys():

[PATCH]romfs: change the mechanism that builds the romtool utility

[Stefan Reinauer]

On 04.04.2009 11:50 Uhr, Patrick Georgi wrote:
> That whole discussion is so unrelated to the actual issue the patch
> attempts to solve, it isn't funny.
>
> To reduce the impact of the patch, here's an updated version that
> doesn't change the behaviour in util/romtool at all.
> The only thing it fixes is the concurrency issue that currently kills
> the first target in the list of the autobuilder.
>
> Feel free to continue to debate the merit of default install locations
> etc (btw: Solaris wants SVr4 style, ie /opt/coreboot/bin or so.
> /usr/local/bin is not part of their file system standard. Please take
> that into account, if you really want to go down that route), I don't
> think romfs should be considered stable (in terms of interfaces and
> file formats) enough for installation yet, so I'll stay away from that.
>
> Again, the patch is probably not whitespace clean (copy&pasted), but I
> will commit as soon as I get an ack.
>
> Signed-off-by: Patrick Georgi

Acked-by: Stefan Reinauer
> Index: util/newconfig/config.g
>
> --- util/newconfig/config.g     (Revision 4060)
> +++ util/newconfig/config.g     (Arbeitskopie)
> @@ -2225,13 +2225,14 @@
>         writemakefileheader(file, makefilepath)
>     # main rule
>   file.write("\nall: romtool")
>   file.write("\nall: ")
>   for i in buildroms:
>           file.write(" %sfs" % i.name)
>   file.write("\n\n")
>
>   # romtool rules
>   file.write("\nromtool:\n\tcd $(TOP)/util/romtool; make\n")
>   file.write("\nromtool:\n\t$(MAKE) -C
>
> $(TOP)/util/romtool\n\tmkdir -p tools\n")
>   file.write("\tcp $(TOP)/util/romtool/tools/rom-mkpayload
>
> $(TOP)/util/romtool/tools/rom-mkstage tools\n\tcp
> $(TOP)/util/romtool/romtool romtool\n")
>     file.write("include Makefile.settings\n\n")
>     for i, o in romimages.items():
>
> @@ -2268,15 +2269,15 @@
>     romsize = getoption("ROM_SIZE", image)
>     # i.name? That can not be right, can it?
>   file.write("%sfs: %s $(TOP)/util/romtool/romtool\n"
>
> %(i.name,i.name));
>   file.write("%sfs: %s romtool\n" %(i.name,i.name));
>   file.write("\trm -f coreboot.romfs\n");
>   file.write("\t$(TOP)/util/romtool/romtool %sfs create %s %s
>
> %s.bootblock\n" % (i.name, romsize, bootblocksize, i.name))
>   file.write("\t./romtool %sfs create %s %s %s.bootblock\n" %
>
> (i.name, romsize, bootblocksize, i.name))
>         for i in buildroms:
>                 for j in i.roms:
>                         #failover is a hack that will go away soon.
>                         if (j != "failover") and (rommapping[j] !=
> "/dev/null"):
>                           file.write("\tif [ -f %s/romfs-support
>
> ]; then $(TOP)/util/romtool/romtool %sfs add-payload %s %s/payload
> `cat %s/romfs-support`; fi\n" % (j, i.name, rommapping[j], j, j))
>           file.write("\t $(TOP)/util/romtool/romtool %sfs
>
> print\n" % i.name)
>                           file.write("\tif [ -f %s/romfs-support
>
> ]; then ./romtool %sfs add-payload %s %s/payload `cat
> %s/romfs-support`; fi\n" % (j, i.name, rommapping[j], j, j))
>           file.write("\t ./romtool %sfs print\n" % i.name)
>
>   file.write(".PHONY: all clean romtool")
>   for i in romimages.keys():

[PATCH]romfs: change the mechanism that builds the romtool utility

[Stefan Reinauer]

On 04.04.2009 11:10 Uhr, Peter Stuge wrote:
> Patrick Georgi wrote:
> > > Is it too much for us to ask that the cbfs tool is built and
> > > installed into the build system? I think that would be ok.
> >
> > "into the build system"?
>
> "on the build system" then.
> > With this patch, it's built in util/romtools and then "installed"
> > into the coreboot-build/*/ directories.
>
> I meant build the tool, then install to /usr/local/bin
> I like that for romcc as well.

This -- imho -- makes little sense for a tool that roughly takes about a
second to compile and serves no other purpose than being a build
requirement for coreboot. It would require us to do version checks of
the utilities, maintain feature lists of working versions, etc. What a
nightmare. Why would we even consider this? Then where would we look for
the binaries? /usr/local/bin? /opt/coreboot/bin on non-linux systems?
What about Darwin? What about Windows? I really don't think we want to care.

Besides, this discussion has nothing to do with the bug fixed by
Patrick's patch.
> If that fails, I think it's ok to at least build each of them in
> their util/ directory and run them there.

We ought to keep our source tree clean and only put objects and binaries
to target/.../ but that's as far as we should go.
> It would be trivial if we did not insist on build directories, but I
> don't believe they will go away.

I suppose you meant it is trivial if we use build directories and build
everything in there?
> At any rate, isn't it actually very easy to build all neccessary
> tools in a common place and then depend on these in each board?
>
> That way they would only be built once.

Not sure if I get right what you are suggesting. The build tools romcc
and romfs are only built once per image.
Optimizing 1s per image/target by introducing a whole bunch of new
problems and dependencies is an incredibly bad idea.

Stefan