Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 141640 - Update savedconfig handling
Summary: Update savedconfig handling
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: New packages (show other bugs)
Hardware: All Linux
: High minor (vote)
Assignee: Embedded Gentoo Team
URL: http://gentoo-wiki.com/TinyGentoo
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-07-24 15:24 UTC by Jac Goudsmit
Modified: 2006-08-09 13:50 UTC (History)
0 users

See Also:
Package list:
Runtime testing required: ---


Attachments
add IUSE=savedconfiglocal to save config in build root instead of $ROOT (busybox-1.2.0.ebuild.savedconfiglocal.patch,4.33 KB, patch)
2006-08-03 16:08 UTC, Jac Goudsmit
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jac Goudsmit 2006-07-24 15:24:31 UTC
When using e.g.:

  ROOT=/tinygentoo USE="savedconfig" emerge busybox

The ebuild will save the busybox .config file in:

  ${ROOT}/etc/${PN}/${CHOST}/${PN}-${PV}-${PR}.config

This happens in the src_install() function in the busybox 1.1.3 and 1.2.0 ebuilds. From the message in that function:

  einfo "Saving this build config to /etc/${PN}/${CHOST}/${PN}-${PV}-${PR}.config"

...it would appear that the intention of the ebuild is to save the config file to /etc/busybox (i.e. in the build host's /etc) instead of ${ROOT}/etc/busybox (i.e. in the target /etc). The intended location on the build host root is (arguably) better because in a cross-development situation such as with TinyGentoo (see URL), we want information such as this .config file to be persistent even when someone decides to empty the target directory (i.e. ${ROOT}) and start over. Besides, in a cross-development situation especially with Busybox, it's more than likely that the target won't even be able to recompile Busybox using the .config file so the file is completely useless to the target.

Sorry I don't know quite enough about Portage (yet) to suggest a fix. I posted a workaround on the discussion page of the TinyGentoo wiki page: after doing any "USE=savedconfig emerge busybox", the user can do the equivalent of:

  mv ${ROOT}/etc/busybox /etc/
Comment 1 solar (RETIRED) gentoo-dev 2006-07-24 19:13:34 UTC
SpankY:
do you view this as a valid bug? My understanding on why we check 
both $ROOT and / is that we are allowing for the max level of customization.
Comment 2 Jac Goudsmit 2006-07-25 09:24:12 UTC
(In reply to comment #1)
Like I said, it gives a message saying "your config is saved in /" and the code looks as if it wants to save it in / too, so it looks to me as if the programmer (you?) had intended it to go into /, not $ROOT. Even if we ignore the question whether / is a better location than $ROOT, the file doesn't go where the script _says_ it goes, so I'd say that's a bug, yes.

Checking for the file in both locations is a good thing and shouldn't be changed.

If you feel that changing the config-saving functionality of the "savedconfig" keyword will break backwards compatibility too much, why not change just the message ("Your config has been saved to...") and add an additional IUSE keyword ("savedconfiglocal") that has the same functionality as "savedconfig" but stores the config in / instead of $ROOT.

I'd be happy to edit the ebuild myself and give you a diff but I wouldn't know how to make Portage use / instead of $ROOT in src_install().

Thanks!
Comment 3 Jac Goudsmit 2006-08-02 17:15:36 UTC
Hmmmm... I finally had some time to learn more about how ebuilds work, and it appears that what I'm asking for, is impossible to do. Correct me if I'm wrong, all destination locations (e.g. $DESTTREE, $INSDESTTREE) are relative to $D and hence relative to $ROOT, right? If so, then the only way to do it is to circumvent portage to write the file (probably causing a sandbox violation).

Setting resolution to CANTFIX for now; I'd still like to resolve this for my particular case (i.e. building a TinyGentoo and keeping the config file in the build host's /etc) but I'll file a new bug report if I can think of something to do that.
Comment 4 Jac Goudsmit 2006-08-03 16:08:56 UTC
Created attachment 93382 [details, diff]
add IUSE=savedconfiglocal to save config in build root instead of $ROOT

This patch implements the savedconfiglocal USE-flag for busybox 1.2.0. Also, the patch implements a change to how the config file is written back at the end of the build: if savedconfig or savedconfiglocal is USEd and an existing config file was found, the ebuild writes the config back to the existing file, not to the default location as the old ebuild did.

The patch should also work on the ebuild of busybox 1.1.3.

Overview of the differences described in the patch:
- IUSE has new keyword savedconfiglocal; it has the same meaning as savedconfig but the location where the config file is saved is in / instead of $ROOT when using savedconfiglocal.
- The definitions of SAVEDCONFIGDESTDIR and SAVEDCONFIGLINK were added to the start of the file with some comments, for clarity
- All "use savedconfig" in the original ebuild were replaced by "(use savedconfig || use savedconfiglocal)".
- Added some sanity checks to the block of code that copies the existing config file if savedconfig || savedconfiglocal is used, so that (file)namespace collisions are detected and will kill the ebuild instead of cause future problems. (See rationale below for using work dir for this)
- When the config file is copied from existing location to source directory, a symlink is also created; this is needed to copy the config file back after the build. The code also checks if the symlink is created successfully. (See rationale below for using work dir)
- The code to save the config file after the build was moved from src_install() to pkg_postinst, so that the script can write outside the sandbox
- The pkg_postinst previously displayed a message about the possibility of using user defined configs. Now it only displays the message if savedconfig || savedconfiglocal aren't given. Otherwise, the script writes the config back via the symlink (see rationale below) if it already existed, or it uses mkdir -p and cp to copy it to the default location which depends on whether savedconfiglocal or savedconfig was given.


Rationale for writing the symlink-related code the way I did:
- First I tried to simply check ${configfile} in pkg_postinst(), this doesn't work because it's no longer defined by that time.
- I tried to export the variable (I know, not a good idea because of possible namespace collision), this didn't work either. Looks like src_install is executed from a sub-shell :-)
- I tried to do ln -s /path/to/.config .config, this doesn't work because during the make process the file is replaced and rewritten as a file, so the pkg_postinst directory can't detect with [[ -L ]] whether the .config was preexisting. (I leave it to the reader to find out the various reasons why a hardlink wouldn't work either)
- I tried to create a symlink or file in the temp directory ${T} but this didn't work either: by the time pkg_postinst needs the file, it's gone (or the path has changed).
Comment 5 Jac Goudsmit 2006-08-03 16:13:36 UTC
Reopening with attached patch as proposal to implement savedconfiglocal IUSE flag (see above); the patch also writes the config back to previously found location (if any) instead of default location.
Comment 6 solar (RETIRED) gentoo-dev 2006-08-03 16:19:04 UTC
SpanKY:
Adding this one is your call. I'm content with the existing behaviors.
Comment 7 SpanKY gentoo-dev 2006-08-05 13:04:57 UTC
config files are now stored in:
/etc/portage/savedconfig/

the file names checked are:
${PF}
${P}
${PN}
${PF}-${CHOST}
${P}-${CHOST}
${PN}-${CHOST}

with .config extensions
Comment 8 Jac Goudsmit 2006-08-06 13:51:02 UTC
(In reply to comment #7)
> config files are now stored in:
> /etc/portage/savedconfig/

Thanks for the change, SpanKY, but I have one question and I think you introduced 2 possible bugs:

Question: I noticed that the code to write the config is still in src_install() so it gets stored under $ROOT. Do I understand correctly that this means you disagree with my statement that it should probably go into / (regardless of setting of $ROOT) in most cases?

Problem 1: storing the config in a different directory (/etc/portage/savedconfig vs. /etc/busybox) and not searching in the old location means that this ebuild will silently break for those people who already had a saved config file and now want to rebuild busybox after an emerge --sync: they will get a default configuration instead of the one they previously saved. I propose that the old locations {${ROOT},}/etc/busybox be added to the search code.

Problem 2: I noticed that you now have two nested for loops with one "break" in it. The old code was a for loop with an "if" (not a for) to test the two alternative paths or filenames. This has some influence on program flow in case a config file exists in multiple locations: the "break" was probably intended to make the cp command happen just once and then break out of the loop (there was only one loop), but now that there are nested loops, I think the break is intended to leave _all_ loops (not just the inner one) so it's still possible for the cp to happen multiple times. If anything, I would say this code's functionality is ambiguous i.e. I'm not sure if it does what it's intended to do. By the way, the old code also checked the various locations in a different sequence from the new code if I remember correctly.

Instead of using the break statement, we could just move the if/einfo/make/return into the for loop as I will show below.

I propose the following rewrite to that for loop, so that the locations are checked in the following sequence, the first found file (and no other files) should be copied:

${ROOT}/etc/portage/savedconfig/${PF}-${CHOST}
${ROOT}/etc/busybox/${PF}-${CHOST}
${ROOT}/etc/portage/savedconfig/${P}-${CHOST}
${ROOT}/etc/busybox/${P}-${CHOST}
${ROOT}/etc/portage/savedconfig/${PN}-${CHOST}
${ROOT}/etc/busybox/${PN}-${CHOST}
/etc/portage/savedconfig/${PF}
/etc/busybox/${PF}
/etc/portage/savedconfig/${P}
/etc/busybox/${P}
/etc/portage/savedconfig/${PN}
/etc/busybox/${PN}

Code fragment proposal: (not tested)
for conf in {${PF},${P},${PN}}{-${CHOST},} ; do
  for root in "${ROOT}" / ; do
    for path in ${root}etc/{portage/savedconfig,busybox} ; do
      if [[ ! -r ${S}/.config ]] ; then
        configfile=${path}/${conf}.config
        if [[ -r ${configfile} ]] ; then
          cp ${configfile} ${S}/.config
          einfo "Found your ${configfile} and using it."
          yes "" | make oldconfig > /dev/null
          return 0 # Maybe we should change this to: return $?
        fi
      fi
    done
  done
done
if [[ ! -r ${S}/.config ]] ; then
  ewarn "No existing config file found in known locations; building a default config"
fi
# Followed by the fi that belongs to if use savedconfig
Comment 9 Jac Goudsmit 2006-08-06 13:55:23 UTC
(In reply to comment #8)

Alright I was solving the problem in two ways at the same time, sorry for the confusion. Here's that code fragment again with one unnecessary "if" removed:

> for conf in {${PF},${P},${PN}}{-${CHOST},} ; do
>   for root in "${ROOT}" / ; do
>     for path in ${root}etc/{portage/savedconfig,busybox} ; do
        configfile=${path}/${conf}.config
        if [[ -r ${configfile} ]] ; then
          cp ${configfile} ${S}/.config
          einfo "Found your ${configfile} and using it."
          yes "" | make oldconfig > /dev/null
          return 0 # Maybe we should change this to: return $?
        fi
>     done
>   done
> done
> if [[ ! -r ${S}/.config ]] ; then
>   ewarn "No existing config file found in known locations; building a default
> config"
> fi
> # Followed by the fi that belongs to if use savedconfig

Comment 10 SpanKY gentoo-dev 2006-08-06 19:49:39 UTC
/etc/busybox is dead, i'm not going to support that anymore

ive fixed the nested loop as you suggested, and made the build abort if the user config file is not located

as for the / vs $ROOT, each has its own advantage ... for now, i'm keeping it with the binpkg
Comment 11 solar (RETIRED) gentoo-dev 2006-08-07 09:10:25 UTC
Not sure we should die if no .config is found. My orig intention with this feature 
was that when no .config was found that we save the one used by the current build.
This is more or less how it was supposed be seeded for the very first time.
Comment 12 Jac Goudsmit 2006-08-07 09:56:45 UTC
Okay, so now if someone has a config in */etc/busybox it won't be found and the build will die. That's cool.

However, again, two problems arise:

First: you're using ${PVR}.config as save-filename. This expands to 1.2.1.config, (not busybox-1.2.1.config). You probably want to use ${PF} instead of ${PVR}.

Second: the ebuild now doesn't save the default config anymore when an existing config was not found (it dies -- which is good, see my first remark). And when a config _IS_ found, it's written back to $ROOT/etc/portage/savedconfig after the build. So now, if I save my config as /etc/portage/savedconfig/busybox.config, it will get used once and will be copied to $ROOT/etc/portage/savedconfig/busybox-1.2.1.config (assuming you will fix the ${PVR} -> ${PF} bug), which will be found first on the next "USE=savedconfig emerge busybox", after which my old config file will be ignored because the new config will be found first.

I'm starting to think, maybe the ebuild shouldn't _write_ the config file at all. Consider this:

In order to use a modified configuration, I have to use FEATURES=keepwork with the emerge (so the work directory won't get cleaned), then go to /var/tmp/portage/busybox*/work, then do a "make menuconfig" to change the config, and then I _HAVE_ to manually copy the config file to */etc/portage/savedconfig so it will get picked up when I do USE=savedconfig emerge busybox again to build my modified busybox; if I don't, the next emerge (with or without savedconfig) will overwrite the modified config in the work dir.

Also I think the build process doesn't really make any significant modifications to the config file (at least I sure hope not, I didn't actually check!) so I don't think there's a need to write the configuration back.

Finally, if the ebuild wouldn't write the config file, it would solve my problem that it always gets written to $ROOT which is undesirable when cross-compiling. That would be perfect :-)
Comment 13 solar (RETIRED) gentoo-dev 2006-08-07 11:16:16 UTC
It really should use PORTAGE_CONFIGROOT vs ROOT these days. And the writeback was a basic part of the intention of this feature. 
The writeback is rather key.
Comment 14 SpanKY gentoo-dev 2006-08-07 18:42:58 UTC
yeah, i meant to convert that to PF instead of PVR ... fixed in cvs

ive also converted to PORTAGE_CONFIGROOT as solar said
Comment 15 Jac Goudsmit 2006-08-08 10:49:11 UTC
Thank you SpanKY.

But I'm afraid another issue has popped up now: The for-loops that search for existing config files should search in ${PORTAGE_CONFIGROOT} too, probably before trying ${ROOT} and / .

And of course there's Solar's problem that if an existing config is not found, the default config is not seeded. Personally I think this is a non-issue because as I pointed out before, the most likely scenario is that users will want to use a make menuconfig in the work dir, so they will have to copy their config file manually anyway.

Other than not searching in ${PORTAGE_CONFIGROOT}, the ebuild now does exactly what I want :-)
Comment 16 SpanKY gentoo-dev 2006-08-08 19:11:56 UTC
ok, src_unpack now searches configroot first

thanks for being so patient with my lame ass ;)
Comment 17 Jac Goudsmit 2006-08-09 10:50:53 UTC
> thanks for being so patient with my lame ass ;)

No way, thank YOU for putting in all that work and listening to my nagging ;-).

I noticed you changed the die-on-not-found to an ewarn, that should make Solar happy too.

One last thing, would you mind moving the write-back code from pkg_preinst() to pkg_postinst() ? The text output generated by pkg_preinst easily scrolls off the screen (and out of my shift-pageup buffer) and pkg_postbuild's einfos kinda look as if nothing happened.

Also I propose to actually put some documentation in the ebuild about using the savedconfig USE-flag. How about this (not tested but you get the idea):

# text to be tabified
# pkg_preinst() to be removed
pkg_postinst() {
  if use savedconfig ; then
    local config_dir="${PORTAGE_CONFIGROOT:-${ROOT}}/etc/portage/savedconfig
    einfo "Saving this build config to ${config_dir}/${PF}.config"
    einfo "Read this ebuild for more info on how to take advantage of this option"
    mkdir -p "${config_dir}"
    cp "${S}/.config" "${config_dir}/${PF}.config"
  else
    echo
    einfo "This ebuild has support for user-defined configs"
    einfo "Please read this ebuild for more details and re-emerge as needed"
    einfo "if you want to add or remove functionality for ${PN}"
    echo
  fi
}

# BUSYBOX ALTERNATE CONFIG MINI-HOWTO
#
# Busybox can be modified in many different ways. Here's one way to do it:
#
# (1) Emerge busybox with FEATURES=keepwork so the work directory won't
#     get erased afterwards. Add a definition like ROOT=/my/root/path to the
#     start of the line if you're installing to somewhere else than the root
#     directory. This command will save the default configuration to
#     ${PORTAGE_CONFIGROOT} (or ${ROOT} if ${PORTAGE_CONFIGROOT} is not 
#     defined), and it will tell you that it has done this. Note the location
#     where the config file was saved.
#
#     FEATURES=keepwork USE=savedconfig emerge busybox
#
# (2) Go to the work directory and change the configuration of busybox using its
#     menuconfig feature.
#
#     cd /var/tmp/portage/busybox*/work
#     make menuconfig
#
# (3) Save your configuration to the default location and copy it to the
#     savedconfig location as follows. Replace X.X.X by the version of 
#     busybox, and change the path if you're overriding ${ROOT} or
#     ${PORTAGE_CONFIGROOT}. The file should overwrite the default config
#     file that was written by the ebuild during step 1.
#
#     cp .config /etc/portage/savedconfig/busybox-X.X.X.config
#
# (4) Execute the same command as in step 1 to build the new busybox config;
#     the FEATURES=keepwork option is probably no longer necessary unless you
#     want to modify the configuration further.
#
Comment 18 solar (RETIRED) gentoo-dev 2006-08-09 11:19:49 UTC
(In reply to comment #17)
> I noticed you changed the die-on-not-found to an ewarn, that should make Solar
> happy too.

Yeah well I updated that when I woke up this morn.. See the ChangeLog.

I also just added your example use to the ebuild as well as shifting preinst to 
postinst.
Comment 19 Jac Goudsmit 2006-08-09 13:50:08 UTC
> I also just added your example use to the ebuild as well as shifting preinst to 
> postinst.

Cool! Thanks.

This bug can be closed as far as I'm concerned.