Re: [PATCH] Re-enable grub-extras

[Vladimir 'φ-coder/phcoder' Serbinenko]

Go ahead for both trunk and extras
On 09/23/2010 03:08 PM, Colin Watson wrote:
> On Tue, Sep 21, 2010 at 07:35:30PM +0100, Colin Watson wrote:
>   
>> Here's a patch to re-enable grub-extras by allowing extra modules to
>> provide Autogen definitions files.  A few things to note:
>>     
> Vladimir commented on this on IRC, noting three problems.  In reverse
> order of complexity:
>
> 1) The GRUB_CONTRIB check in autogen.sh should also check for
>    grub-core/contrib.
>
> Fixed.
>
> 2) What's the problem with renaming g2hdr.bin to g2hdr.img?
>
> I know of at least one piece of software which expects the current file
> name: win32-loader, written by Robert and maintained in Debian.  It
> probably wouldn't be completely awful to change this, but it would be
> nice not to need to.  That said, perhaps g2hdr.bin could be a symlink to
> g2hdr.img (which could be done in Makefile.common).  I've updated my
> patch to do this, and removed the extension-override facility from
> gentpl.py.
>
> 3) It would be nice if extra modules could add new sources to existing
>    targets defined in one of the main definitions files: for example, it
>    would be useful to be able to add zfs to libgrub.a.
>
> This was the hard bit that took me a day or two to fix.
>
> Vladimir's original suggestion on IRC was to create a separate
> libgrubzfs.a and arrange to link the utilities with that as well.  When
> I looked into this, I decided that I didn't like this approach because
> it simply pushes the problem of appending to an existing list up a
> level; as far as the Autogen definitions go, there isn't a significant
> difference between appending to grub_probe_LDADD and appending to
> libgrub_a_SOURCES.  I would prefer to solve this some other way.
>
> My preferred approach is to try not to expose any of the difficulties in
> the definitions file, so that it can just look like this:
>
>   library = {
>     name = libgrub.a;
>     common = contrib/zfs/zfs.c;
>     common = contrib/zfs/zfs_lzjb.c;
>     common = contrib/zfs/zfs_sha256.c;
>     common = contrib/zfs/zfs_fletcher.c;
>     cppflags = '$(ZFS_CPPFLAGS)';
>     cflags = '$(ZFS_CFLAGS)';
>   };
>
> Doing this actually turned out to be very difficult because Autogen's
> native macros aren't powerful enough: they can only really emit text
> rather than using temporary storage, which means that you can't do
> things like "look for all the library blocks with the same name and
> concatenate them" or "append to _SOURCES variables if we've seen them
> before".  Some Automake variables can be handled with simple appends
> while some of them have some boilerplate only at the start which
> shouldn't be repeated; Automake variables must be initialised exactly
> once on all possible intersections of conditional paths; and we should
> also avoid generating duplicate rules.  All these constraints make it a
> hard problem.
>
> Fortunately, Autogen supports embedding bits of Guile, which makes the
> problem merely hard rather than impossible.  I tried a few different
> approaches, and the one that I found to be most practical is in the
> following patch: effectively we remember whether we've seen a given
> target before and treat it slightly differently, and furthermore we emit
> initialisations for non-global variables to avoid problems with
> Automake's restrictions on variable initialisation.
>
> For now, I only implemented this for 'library' blocks, so something that
> wants to compile extra files into the GRUB kernel for example will be
> out of luck.  If people feel this is an onerous restriction then it's
> fairly easy to fix; I was just trying not to change too much at once,
> particularly for paths I wasn't going to test.
>
> Doing this involved feeding all the files relevant to a given Automake
> input stream to Autogen in one go, so I eliminated Makefile.gcry.am.
>
> This was an exercise in programming in six languages at once (Python,
> Autogen native macros, Guile, Autogen definitions, Automake, and make).
> I found it to be a bit much, and my impression is that Autogen got in
> the way more than it helped.  I suggest that after 1.99 we should look
> at having a Python program generate Automake input directly, which would
> avoid many contortions and allow us to improve the output (it could
> merge the multiple _SOURCES lines into just one per conditional path,
> for instance).  The .def file format is of course just fine as it is,
> and could easily be preserved.
>
>   
>>   * Extra modules may also provide Makefile.common with literal Automake
>>     input, for those cases where it's too hard to cram things into
>>     Autogen.
>>     
> I also added Makefile.core.common and Makefile.util.common (starting to
> run out of good names) for literal Automake input that should only be
> used in grub-core or at the top level respectively.  ntldr-img needed
> the former.
>
> Once again, the required patches to grub-extras modules are attached.
>
> 2010-09-22  Colin Watson  <address@hidden>
>
>       Re-enable grub-extras.
>
>       * autogen.sh: Create symlinks to ${GRUB_CONTRIB} if necessary to
>       avoid confusing Automake.  Run autogen only twice, once for the top
>       level and once for grub-core.  Add Makefile.util.def and
>       Makefile.core.def from extra modules to the appropriate autogen
>       invocations.  If Makefile.common exists in an extra module, include
>       it in both Makefile.util.am and grub-core/Makefile.core.am;
>       similarly, include any Makefile.util.common file in Makefile.util.am
>       and any Makefile.core.common file in grub-core/Makefile.core.am.
>       * conf/Makefile.common ($(top_srcdir)/grub-core/Makefile.core.am):
>       Depend on $(top_srcdir)/grub-core/Makefile.gcry.def.
>       ($(top_srcdir)/grub-core/Makefile.gcry.def): Remove.
>       * grub-core/Makefile.am: Remove inclusion of Makefile.gcry.am.
>
>       * gentpl.py (gvar_add): Turn GVARS into a set.
>       (global_variable_initializers): Sort global variables on output.
>       (vars_init): New function.
>       (first_time): Likewise.
>       (library): Ensure that non-global variable initialisations are
>       emitted before the first time we emit code for a library block.
>       Append to variables rather than setting them.  Only emit
>       noinst_LIBRARIES, BUILT_SOURCES, and CLEANFILES the first time for
>       each conditional path.
>       (program): installdir() emits an Autogen macro, so must be passed to
>       var_add rather than gvar_add.
>       (data): Likewise.
>       (script): Likewise.
>       (rules): New function, centralising handling for different target
>       types.  Set up Guile association lists for first_time and vars_init,
>       and send most output to a diversion so that variable initialisations
>       can be emitted first.
>       (module_rules): Use new rules function.
>       (kernel_rules): Likewise.
>       (image_rules): Likewise.
>       (library_rules): Likewise.
>       (program_rules): Likewise.
>       (script_rules): Likewise.
>       (data_rules): Likewise.
>
>       * configure.ac: Add AC_PROG_LN_S, for the benefit of ntldr-img.
>
>       * .bzrignore: Add contrib and grub-core/contrib.  Remove
>       grub-core/Makefile.gcry.am.
>
> === modified file '.bzrignore'
> --- .bzrignore        2010-09-20 13:03:47 +0000
> +++ .bzrignore        2010-09-22 13:07:15 +0000
> @@ -104,9 +104,10 @@ grub-core/lib/libgcrypt-grub
>  **/.deps-core
>  **/.dirstamp
>  Makefile.util.am
> +contrib
>  grub-core/Makefile.core.am
> -grub-core/Makefile.gcry.am
>  grub-core/Makefile.gcry.def
> +grub-core/contrib
>  grub-core/genmod.sh
>  grub-core/gensyminfo.sh
>  grub-core/*.module
>
> === modified file 'autogen.sh'
> --- autogen.sh        2010-08-23 08:37:29 +0000
> +++ autogen.sh        2010-09-23 10:36:52 +0000
> @@ -14,9 +14,50 @@ echo "Creating Makefile.tpl..."
>  python gentpl.py | sed -e '/^$/{N;/^\n$/D;}' > Makefile.tpl
>  
>  echo "Running autogen..."
> -autogen -T Makefile.tpl Makefile.util.def | sed -e '/^$/{N;/^\n$/D;}' > 
> Makefile.util.am
> -autogen -T Makefile.tpl grub-core/Makefile.core.def | sed -e 
> '/^$/{N;/^\n$/D;}' > grub-core/Makefile.core.am
> -autogen -T Makefile.tpl grub-core/Makefile.gcry.def | sed -e 
> '/^$/{N;/^\n$/D;}' > grub-core/Makefile.gcry.am
> +
> +# Automake doesn't like including files from a path outside the project.
> +rm -f contrib grub-core/contrib
> +if [ "x${GRUB_CONTRIB}" != x ]; then
> +  [ "${GRUB_CONTRIB}" = contrib ] || ln -s "${GRUB_CONTRIB}" contrib
> +  [ "${GRUB_CONTRIB}" = grub-core/contrib ] || ln -s ../contrib 
> grub-core/contrib
> +fi
> +
> +UTIL_DEFS=Makefile.util.def
> +CORE_DEFS='grub-core/Makefile.core.def grub-core/Makefile.gcry.def'
> +
> +for extra in contrib/*/Makefile.util.def; do
> +  if test -e "$extra"; then
> +    UTIL_DEFS="$UTIL_DEFS $extra"
> +  fi
> +done
> +
> +for extra in contrib/*/Makefile.core.def; do
> +  if test -e "$extra"; then
> +    CORE_DEFS="$CORE_DEFS $extra"
> +  fi
> +done
> +
> +cat $UTIL_DEFS | autogen -T Makefile.tpl | sed -e '/^$/{N;/^\n$/D;}' > 
> Makefile.util.am
> +cat $CORE_DEFS | autogen -T Makefile.tpl | sed -e '/^$/{N;/^\n$/D;}' > 
> grub-core/Makefile.core.am
> +
> +for extra in contrib/*/Makefile.common; do
> +  if test -e "$extra"; then
> +    echo "include $extra" >> Makefile.util.am
> +    echo "include $extra" >> grub-core/Makefile.core.am
> +  fi
> +done
> +
> +for extra in contrib/*/Makefile.util.common; do
> +  if test -e "$extra"; then
> +    echo "include $extra" >> Makefile.util.am
> +  fi
> +done
> +
> +for extra in contrib/*/Makefile.core.common; do
> +  if test -e "$extra"; then
> +    echo "include $extra" >> grub-core/Makefile.core.am
> +  fi
> +done
>  
>  echo "Saving timestamps..."
>  echo timestamp > stamp-h.in
>
> === modified file 'conf/Makefile.common'
> --- conf/Makefile.common      2010-09-21 12:37:50 +0000
> +++ conf/Makefile.common      2010-09-22 13:09:44 +0000
> @@ -146,11 +146,7 @@ $(top_srcdir)/Makefile.util.am: $(top_sr
>       mv address@hidden $@
>  
>  .PRECIOUS: $(top_srcdir)/grub-core/Makefile.core.am
> -$(top_srcdir)/grub-core/Makefile.core.am: 
> $(top_srcdir)/grub-core/Makefile.core.def $(top_srcdir)/Makefile.tpl
> -     autogen -T $(top_srcdir)/Makefile.tpl $< | sed -e '/^$$/{N;/^\\n$$/D;}' 
> > address@hidden || (rm -f address@hidden; exit 1)
> -     mv address@hidden $@
> -
> -.PRECIOUS: $(top_srcdir)/grub-core/Makefile.gcry.am
> -$(top_srcdir)/grub-core/Makefile.gcry.am: 
> $(top_srcdir)/grub-core/Makefile.gcry.def $(top_srcdir)/Makefile.tpl
> -     autogen -T $(top_srcdir)/Makefile.tpl $< | sed -e '/^$$/{N;/^\\n$$/D;}' 
> > address@hidden || (rm -f address@hidden; exit 1)
> +$(top_srcdir)/grub-core/Makefile.core.am: 
> $(top_srcdir)/grub-core/Makefile.core.def 
> $(top_srcdir)/grub-core/Makefile.gcry.def $(top_srcdir)/Makefile.tpl
> +     if [ "x$$GRUB_CONTRIB" != x ]; then echo "You need to run ./autogen.sh 
> manually." >&2; exit 1; fi
> +     autogen -T $(top_srcdir)/Makefile.tpl 
> $(top_srcdir)/grub-core/Makefile.core.def 
> $(top_srcdir)/grub-core/Makefile.gcry.def | sed -e '/^$$/{N;/^\\n$$/D;}' > 
> address@hidden || (rm -f address@hidden; exit 1)
>       mv address@hidden $@
>
> === modified file 'configure.ac'
> --- configure.ac      2010-09-21 09:42:30 +0000
> +++ configure.ac      2010-09-22 12:11:35 +0000
> @@ -235,6 +235,7 @@ AC_PROG_LEX
>  AC_PROG_YACC
>  AC_PROG_MAKE_SET
>  AC_PROG_MKDIR_P
> +AC_PROG_LN_S
>  
>  if test "x$LEX" = "x:"; then
>    AC_MSG_ERROR([flex is not found])
>
> === modified file 'gentpl.py'
> --- gentpl.py 2010-09-19 13:24:45 +0000
> +++ gentpl.py 2010-09-22 21:01:22 +0000
> @@ -70,16 +70,15 @@ for platform in GRUB_PLATFORMS:
>  #
>  # Global variables
>  #
> -GVARS = []
> +GVARS = set()
>  
>  def gvar_add(var, value):
> -    if var not in GVARS:
> -        GVARS.append(var)
> +    GVARS.add(var)
>      return var + " += " + value + "\n"
>  
>  def global_variable_initializers():
>      r = ""
> -    for var in GVARS:
> +    for var in sorted(GVARS):
>          r += var + " ?= \n"
>      return r
>  
> @@ -87,6 +86,16 @@ def global_variable_initializers():
>  # Per PROGRAM/SCRIPT variables 
>  #
>  
> +def vars_init(*var_list):
> +    r = "[+ IF (if (not (assoc-ref seen-vars (get \".name\"))) \"seen\") +]"
> +    r += "[+ (out-suspend \"v\") +]"
> +    for var in var_list:
> +        r += var + "  = \n"
> +    r += "[+ (out-resume \"v\") +]"
> +    r += "[+ (set! seen-vars (assoc-set! seen-vars (get \".name\") 0)) +]"
> +    r += "[+ ENDIF +]"
> +    return first_time(r)
> +
>  def var_set(var, value):
>      return var + "  = " + value + "\n"
>  
> @@ -257,6 +266,15 @@ def platform_ccasflags(p): return platfo
>  def platform_stripflags(p): return platform_specific_values(p, 
> "_stripflags", "stripflags")
>  def platform_objcopyflags(p): return platform_specific_values(p, 
> "_objcopyflags", "objcopyflags")
>  
> +#
> +# Emit snippet only the first time through for the current name.
> +#
> +def first_time(snippet):
> +    r = "[+ IF (if (not (assoc-ref seen-target (get \".name\"))) \"seen\") 
> +]"
> +    r += snippet
> +    r += "[+ ENDIF +]"
> +    return r
> +
>  def module(platform):
>      r = set_canonical_name_suffix(".module")
>  
> @@ -341,18 +359,25 @@ fi
>  
>  def library(platform):
>      r = set_canonical_name_suffix("")
> -    r += gvar_add("noinst_LIBRARIES", "[+ name +]")
> -    r += var_set(cname() + "_SOURCES", platform_sources(platform))
> -    r += var_set("nodist_" + cname() + "_SOURCES", 
> platform_nodist_sources(platform))
> -    r += var_set(cname() + "_CFLAGS", "$(AM_CFLAGS) $(CFLAGS_LIBRARY) " + 
> platform_cflags(platform))
> -    r += var_set(cname() + "_CPPFLAGS", "$(AM_CPPFLAGS) $(CPPFLAGS_LIBRARY) 
> " + platform_cppflags(platform))
> -    r += var_set(cname() + "_CCASFLAGS", "$(AM_CCASFLAGS) 
> $(CCASFLAGS_LIBRARY) " + platform_ccasflags(platform))
> -    # r += var_set(cname() + "_DEPENDENCIES", 
> platform_dependencies(platform) + " " + platform_ldadd(platform))
>  
> -    r += gvar_add("EXTRA_DIST", extra_dist())
> -    r += gvar_add("BUILT_SOURCES", "$(nodist_" + cname() + "_SOURCES)")
> -    r += gvar_add("CLEANFILES", "$(nodist_" + cname() + "_SOURCES)")
> +    r += vars_init(cname() + "_SOURCES",
> +                   "nodist_" + cname() + "_SOURCES",
> +                   cname() + "_CFLAGS",
> +                   cname() + "_CPPFLAGS",
> +                   cname() + "_CCASFLAGS")
> +    #              cname() + "_DEPENDENCIES")
>  
> +    r += first_time(gvar_add("noinst_LIBRARIES", "[+ name +]"))
> +    r += var_add(cname() + "_SOURCES", platform_sources(platform))
> +    r += var_add("nodist_" + cname() + "_SOURCES", 
> platform_nodist_sources(platform))
> +    r += var_add(cname() + "_CFLAGS", first_time("$(AM_CFLAGS) 
> $(CFLAGS_LIBRARY) ") + platform_cflags(platform))
> +    r += var_add(cname() + "_CPPFLAGS", first_time("$(AM_CPPFLAGS) 
> $(CPPFLAGS_LIBRARY) ") + platform_cppflags(platform))
> +    r += var_add(cname() + "_CCASFLAGS", first_time("$(AM_CCASFLAGS) 
> $(CCASFLAGS_LIBRARY) ") + platform_ccasflags(platform))
> +    # r += var_add(cname() + "_DEPENDENCIES", 
> platform_dependencies(platform) + " " + platform_ldadd(platform))
> +
> +    r += gvar_add("EXTRA_DIST", extra_dist())
> +    r += first_time(gvar_add("BUILT_SOURCES", "$(nodist_" + cname() + 
> "_SOURCES)"))
> +    r += first_time(gvar_add("CLEANFILES", "$(nodist_" + cname() + 
> "_SOURCES)"))
>      return r
>  
>  def installdir(default="bin"):
> @@ -376,7 +401,7 @@ def program(platform, test=False):
>      r += gvar_add("check_PROGRAMS", "[+ name +]")
>      r += gvar_add("TESTS", "[+ name +]")
>      r += "[+ ELSE +]"
> -    r += gvar_add(installdir() + "_PROGRAMS", "[+ name +]")
> +    r += var_add(installdir() + "_PROGRAMS", "[+ name +]")
>      r += "[+ IF mansection +]" + manpage() + "[+ ENDIF +]"
>      r += "[+ ENDIF +]"
>  
> @@ -397,7 +422,7 @@ def program(platform, test=False):
>  def data(platform):
>      r  = gvar_add("EXTRA_DIST", platform_sources(platform))
>      r += gvar_add("EXTRA_DIST", extra_dist())
> -    r += gvar_add(installdir() + "_DATA", platform_sources(platform))
> +    r += var_add(installdir() + "_DATA", platform_sources(platform))
>      return r
>  
>  def script(platform):
> @@ -405,7 +430,7 @@ def script(platform):
>      r += gvar_add("check_SCRIPTS", "[+ name +]")
>      r += gvar_add ("TESTS", "[+ name +]")
>      r += "[+ ELSE +]"
> -    r += gvar_add(installdir() + "_SCRIPTS", "[+ name +]")
> +    r += var_add(installdir() + "_SCRIPTS", "[+ name +]")
>      r += "[+ IF mansection +]" + manpage() + "[+ ENDIF +]"
>      r += "[+ ENDIF +]"
>  
> @@ -418,33 +443,43 @@ chmod a+x [+ name +]
>      r += gvar_add("dist_noinst_DATA", platform_sources(platform))
>      return r
>  
> +def rules(target, closure):
> +    # Create association lists for the benefit of first_time and vars_init.
> +    r = "[+ (define seen-target '()) +]"
> +    r += "[+ (define seen-vars '()) +]"
> +    # Most output goes to a diversion.  This allows us to emit variable
> +    # initializations before everything else.
> +    r += "[+ (out-push-new) +]"
> +
> +    r += "[+ FOR " + target + " +]"
> +    r += foreach_enabled_platform(
> +        lambda p: under_platform_specific_conditionals(p, closure(p)))
> +    # Remember that we've seen this target.
> +    r += "[+ (set! seen-target (assoc-set! seen-target (get \".name\") 0)) 
> +]"
> +    r += "[+ ENDFOR +]"
> +    r += "[+ (out-pop #t) +]"
> +    return r
> +
>  def module_rules():
> -    return "[+ FOR module +]" + foreach_enabled_platform(
> -        lambda p: under_platform_specific_conditionals(p, module(p))) + "[+ 
> ENDFOR +]"
> +    return rules("module", module)
>  
>  def kernel_rules():
> -    return "[+ FOR kernel +]" + foreach_enabled_platform(
> -        lambda p: under_platform_specific_conditionals(p, kernel(p))) + "[+ 
> ENDFOR +]"
> +    return rules("kernel", kernel)
>  
>  def image_rules():
> -    return "[+ FOR image +]" + foreach_enabled_platform(
> -        lambda p: under_platform_specific_conditionals(p, image(p))) + "[+ 
> ENDFOR +]"
> +    return rules("image", image)
>  
>  def library_rules():
> -    return "[+ FOR library +]" + foreach_enabled_platform(
> -        lambda p: under_platform_specific_conditionals(p, library(p))) + "[+ 
> ENDFOR +]"
> +    return rules("library", library)
>  
>  def program_rules():
> -    return "[+ FOR program +]" + foreach_enabled_platform(
> -        lambda p: under_platform_specific_conditionals(p, program(p))) + "[+ 
> ENDFOR +]"
> +    return rules("program", program)
>  
>  def script_rules():
> -    return "[+ FOR script +]" + foreach_enabled_platform(
> -        lambda p: under_platform_specific_conditionals(p, script(p))) + "[+ 
> ENDFOR +]"
> +    return rules("script", script)
>  
>  def data_rules():
> -    return "[+ FOR data +]" + foreach_enabled_platform(
> -        lambda p: under_platform_specific_conditionals(p, data(p))) + "[+ 
> ENDFOR +]"
> +    return rules("data", data)
>  
>  print "[+ AutoGen5 template +]\n"
>  a = module_rules()
>
> === modified file 'grub-core/Makefile.am'
> --- grub-core/Makefile.am     2010-09-19 20:22:43 +0000
> +++ grub-core/Makefile.am     2010-09-22 13:11:11 +0000
> @@ -51,7 +51,6 @@ grub_script.yy.c: grub_script.yy.h
>  CLEANFILES += grub_script.yy.c grub_script.yy.h
>  
>  include $(srcdir)/Makefile.core.am
> -include $(srcdir)/Makefile.gcry.am
>  
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/cache.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/command.h
>
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature