Re: [gentoo-dev] New Eclasses: postgres and postgres-multi

[Aaron W. Swenson]

On 2016-01-25 10:31, Ian Stakenvicius wrote:
> On 23/01/16 10:51 AM, Ian Stakenvicius wrote:
> >> On Jan 22, 2016, at 3:11 PM, Aaron W. Swenson
> >> <titanofold@g.o> wrote:
> >>
> >> I would like some feedback on the documentation/comments in
> >> the eclass. I'm certain it could be improved. Though, if you
> >> were able to follow them (not a slight, just you were the first
> >> to follow them), I might have done good enough.
> >>
> >
> > I'll have another look; i don't think I read any of the docs,
> > just looked to see that PG_CONFIG was set for me and confirmed
> > things looked to work the same as multilib-minimal.
>
> OK here's my full review (with docs and all):
>
> postgres.eclass:
>
> postgres_check_slot() <-- #1, not seeing a need for this as
> dependencies should be forced now due to USE flag provisions.  #2,
> though, the documentation says it should be used in pkg_pretend but
> then in the code there's an exclusion to make it a no-op when it's
> run in pkg_pretend.  Perhaps it's supposed to be run in pkg_setup??

I'm a bit uncertain what to do with this. There are a few ebuilds that
check that the slot is set properly, and they do so as early as possible
so the user doesn't start the emerge and walk away thinking everything
will be okay. So, it tries to run the check, but won't die if the
PostgreSQL eselect module hasn't been emerged yet.

I could probably replace it with a pkg_setup that sets the PG_CONFIG
variable globally.

But, this would probably require another USE_EXPAND to do a single
target. I'm not against this, I just don't want to go use flag crazy.

> postgres_new_user() <-- the documentation seems a little bit unclear
> on this one..  It looks like it creates the 'postgres' user/group
> and optionally it also creates an additional specified user/group..
>  But i'm not sure how creating new users/groups here instead of with
> calling enewuser/enewgroup directly in the ebuild is helpful per
> se...?  It also seems a bit redundant if you wanted to create say, 2
> or 3 extra users/groups, to repeatedly call enewuser postgres ....

You're right about how it works. I could scratch the rest of the
function and have it just create the postgres system group and
user. It's intended to make it so that a developer doesn't have to look
up the postgres system user and group parameters in another ebuild, or
dig around for nonexistent documentation, which will be rectified but
that'll still be difficult to search.

> postgres-multi.eclass:
>
> #1 - main description could perhaps be changed from:
>
> # build against any and all compatible PostgreSQL slots that are also
> # enabled by the user.
>
> to:
>
> # build and install for one or more PostgreSQL slots as specified by
> # POSTGRES_TARGETS use flags.

Done.

> #2 - POSTGRES_COMPAT , needs an example and/or text to describe the
> syntax of the slot specification.

Done.

> #3 - _POSTGRES_UNION_SLOTS -- you actually mean here the
> intersection between the two sets, rather than the union, right?
> Var can likely stay as-is but the description should probably be
> updated.

Well, that's a bit embarrassing. You'd think I'd know the difference by
now. Renamed it _POSTGRES_INTERSECT_SLOTS and corrected other
appearances of union.

> #4 - postgres-multi_foreach() -- this actually runs in the builddir,
> not the source dir as mentioned in the text.

Done.

> #5 - postgres-multi_forbest() -- maybe expand on what the "best"
> slot is here, ie that it's the lexicographically biggest slot rather
> than something else (like, say, the slot that's currently eselected)

Done.

> #6 - need docs for postgres-multi_src_prepare() and so forth -- just
> simple stuff will suffice I think:  postgres-multi_src_prepare()
> copies ${S} into a build directory for each target PG_SLOT and
> should be specified; the others are default functions that are
> provided for convenience, but postgres-multi_foreach should be used
> instead of these when customization is necessary.

Done.

> ...and I think that's it..

Thanks!