Profile support and refactoring

[Osamu Aoki]

Hi,

I just joined ML.

On Fri, Jan 08, 2010 at 06:33:18PM -0500, Loïc Minier wrote:
>  Generally, I'd prefer the name "profiles" instead of modes because I
>  find the latter a bit confusing.

Fixed:  --profile is now used.
 
>  You could use ${PBMODE:+$PBMODE/} instead to avoid the extra slash.

Good idea. (Actually, there are many // in path even without this.  This
type of code cleaning may be good idea across pbuilder.)

>  pbuilderrc.5: perhaps we should simply leave the absolute defaults
>  documented and mention that this is changed when using --mode; e.g
... 
>  We could keep:
>    .BI "BUILDRESULT=" "/var/cache/pbuilder/result/"
>  and add a section on modes which says:
>    When using --mode, some defaults settings will be adjusted such as
>    BUILDRESULT, APTCACHE, ...; you may still override these in the usual
>    ways.
>  (just as in pbuilder.8)

I thought about it too.  But using expression like "some defaults
settings" are quite vague.  What is wrong to say "all default settings
containing '/var/cache/pbuilder' will be moved to
'/var/cache/pbuilder/${PBPROFILE}'?

By the way, What do you think about factoring some related variables like:

PBUILDER_CACHEDIR=/var/cache/pbuilder

If this is defined in /usr/share/pbuilder/pbuilderrc and used across
code consistently across pbuilder, it becomes cleaner. (I did not do
this to minimize patch size for easy initial code review.) I will get
back to this below.

>  pbuilder.8:
>     -.BI "pbuilder --create [" "options" "]"
>     +.BI "pbuilder [--mode pbmode] --create [" "options" "]"
>  don't think you actually need to mention that here; just mention the
>  flag later.  We can mention the new feature in the debian/NEWS to get
>  some attention.

But isn't it the purpose of manpage to explain syntax of command here.
Dropping these altogether seems bad idea.  But I understand these looks
crowded.
 
>  pbuilder:
>     +# pbuilder mode option: Osamu Aoki <osamu at debian.org> 2009 Dec 31
>  no need to mention you changed that, it will be recorded by git ("git
>  blame pbuilder").

I had already realized and removed them.

>  pbuilder/pbuilderrc: I think the flag should be shorter because it will
>  be used frequently; how about "-m" for mode, or "-p" for profile?

They are.  As I come to think of it, maybe manpage examples should use
short form as main form.....

 +.BI "pbuilder [--p profile] --create [" "options" "]"

Then it is easier to read and explain.

Actually, I always wondered which syntax is recommended and which is
deprecated backward compatibility.  I.e., "--create" or "create".
Although both seem to be allowed, manpage list one of them.

>  So if you check my initial proposal, I propose *recording* more things
>  in the mode / profile: the target distribution for instance.  I think
>  that "pbuilder -p foo create --distribution squeeze" should create a
>  foo.pbuilderrc which sets DISTRIBUTION=squeeze so that subsequent
>  commands keep using that (or --mirror, etc.).  What do you think of
>  this part?  Is this something you would be interested in implementing?
>  Or if you have no interest, would you mind if I did it?

Although I mentioned some to BTS, let me go over.

I avoided this in order to make "-p profile" feature is minimal and
easily accepted.  As I said, --configfile option can be used with
"create" so making new feature like this is minimal advantage if one
wishes to tweak /etc/apt/sources.list to add testing-security-updates
etc.  These can be done better by accessing cheroot directly by user or
via hook script using the generic infrastructure.  Too much special
features will not benefit user.

But I do not rule out as long as infrastructure is well thought and
simple. 

Let's see where they are loaded from:

 * pbuilder-loadconfig
   "$PBUILDER_PKGDATADIR"/pbuilderrc
   "$PBUILDER_SYSCONFDIR"/pbuilderrc
   "$HOME"/.pbuilderrc  

 * pbuilder-checkparams (source pbuilder-loadconfig)

 * pdebuild-checkparams (source pbuilder-loadconfig)

 * pbuilder-uml-checkparams (independently defined)
   /usr/share/pbuilder/pbuilder-uml.conf
   /etc/pbuilder/pbuilder-uml.conf 
   ${HOME}/.pbuilderrc
   ${HOME}/.pbuilder-umlrc

 * pdebuild-uml-checkparams (independently defined)
   /usr/share/pbuilder/pbuilder-uml.conf 
   /etc/pbuilder/pbuilder-uml.conf 
   ${HOME}/.pbuilderrc 
   ${HOME}/.pbuilder-umlrc

 * /etc/pbuilder
   it contains /etc/pbuilder/pbuilderrc -> ../pbuilderrc
   (This looks better place to keep real config)

Somewhat inconsistent as I see.  Also having similar code for normal and uml
version is prone for inconsistency.

If we fix this complication in consistent way and profile support is
integrated, then it is worth adding feature like you mention.

As I read source for *-uml, I realize refactoring these configuration loading
scheme may be good idea to make -p work for *-uml.  (I now see *-uml can not
use profile feature yet.)

Her is my idea how this can be solved but this is too much changes unless
everyone agrees and work together quickly.

1. re-factor code using "$PBUILDER_CACHEDIR" etc.

For example:

   1.1 calling program exports following variables:
   
   PBPROFILE=""                                            (if without -p)
   PBPROFILE="profile"                                     (if with -p profile)
   PBUILDER_CACHEDIR=/var/cache/pbuilder                   (if normal pbuilder)
   PBUILDER_CACHEDIR=$HOME/pbuilder/cache                  (if {pbuilder,pdebuild}-uml)
   PBUILDER_PKGDATADIR=/usr/share/pbuilder
   PBUILDER_SYSCONFDIR=/etc/pbuilder

   (Move Current -uml choice is $HOME/tmp to $HOME/pbuilder/cache)

   1.2 standard loading program reads configuration data in the following order

   "$PBUILDER_PKGDATADIR"/pbuilderrc
   "$PBUILDER_SYSCONFDIR"/pbuilder/pbuilderrc
   "$HOME"/.pbuilder/pbuilderrc
   "$PBUILDER_PKGDATADIR"/${PBPROFILE}pbuilderrc           (if -n ${PBPROFILE})
   "$PBUILDER_SYSCONFDIR"/pbuilder/${PBPROFILE}pbuilderrc  (if -n ${PBPROFILE})
   "$HOME"/.pbuilder/${PBPROFILE}pbuilderrc                (if -n ${PBPROFILE})

2. merge *-uml into main code. (next step?)

 pbuilder -p profile [create|update|build|login|clean|execute] [--options]

  options to contain --targz (default), --no-targz, --uml, --unionfs, .... which set 
  PBUILDER_IMAGE_TYPE variable ...

3. Add easy access to build environments.

If we do refactoring as described above, then we can also ship template
configurations for each typical pbuilder scenario/profile:
   /usr/share/pbuilder/sarge/pbuilderrc
   /usr/share/pbuilder/lenny/pbuilderrc
   /usr/share/pbuilder/sid/pbuilderrc
   /usr/share/pbuilder/experimental/pbuilderrc
   /usr/share/pbuilder/kermic/pbuilderrc
   /usr/share/pbuilder/jaunty/pbuilderrc
   ...
   (With proper DISRIBUTION=, MIRROR=,...)

Use ${URL_DEBIAN_ARCHIVE} and ${URL_UBUNTU_ARCHIVE} in these files.
Define as  ${URL_DEBIAN_ARCHIVE:-some default value} and
${URL_UBUNTU_ARCHIVE:-some default value} in
/usr/share/pbuilder/pbuilderrc to allow local customization across all
releases, so we can override it from /etc/pbuilder/pbuilderrc or
$HOME/.pbuilder/pbuilderrc.  (The order of configuration file parsing is
important for this to work.)

Now we can do

$ pbuilder -p hardy create
  ...

$ pbuilder -sarge create --uml

This should be much easier than reading manual and setting all arguments
to pbuilder.

I chacked your RFC.  Most points can be effectively addressed with some
syntax differences for the folllowing:

|  - point to a /etc/pbuilder/$profile.pbuilderrc main config file
|  - adapt pbuilder's pathnames for the tarball and result dirs, e.g.
|    /var/cache/pbuilder/base-$profile.tgz and
|    /var/cache/pbuilder/result-$profile or similar
|  - store custom settings in the $profile.pbuilderrc, notably:
|    * --distribution => DISRIBUTION=, --components etc.
|    *  --mirror => MIRROR=, --othermirror etc.

As for "cross-arch support", I have not thought about it.  I have not
used qemubuilder (comes in cowbuilder source).  If cowbuilder is updated
as the next stepo, this should be updated and may take care this kind of
needs, I guess.

As for "support of multiple APT repos", I think this can get to
complicated and most easily achieved by (as I discussed):
 * copying similar configuration from /usr/share/pbuilder/*/pbuilderrc to
   /etc/pbuilder/*/pbuilderrc 
 * --login --save-after-login and edit as you wish.
 * If automation is needed, --exec or is another way, I guess.

>  Another high level comment is with the first commit message: instead of
>  "Osamu's branch to enable mode support" in the first line, you might
>  want to just state "Add mode support" or something similar.  Also, you
>  could name your branch "mode-support".  I'm happy to help fixing the
>  commit message or branch name if you like.

Please do it with "profile-support" branch after looking at latest code
so the merged code looks clean.  I appreciate your help.  (When you are
removing osamu branch at shared host, give us fair warning.)

Osamu