Sender: =?UTF-8?Q?Timo_Ter=C3=A4s?= <timo.teras@gmail.com>
Date: Fri, 28 Jun 2013 21:00:07 +0300
From: Timo Teras <timo.teras@iki.fi>
To: Dubiousjim <dubiousjim@gmail.com>
Cc: alpine-devel@lists.alpinelinux.org
Subject: Re: [alpine-devel] futher apk ideas (WAS: [PATCH 00/15] apk-tools
 UI fixes/refinements)
Message-ID: <20130628210007.293d7fb2@vostro>
In-Reply-To: <20130628163747.GB1712@zen.nyu.edu>
References: <cover.1372361242.git.dubiousjim@gmail.com>
	<20130628154631.4d18107d@ncopa-desktop.alpinelinux.org>
	<20130628163747.GB1712@zen.nyu.edu>
Precedence: list
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 28 Jun 2013 12:37:47 -0400
Dubiousjim <dubiousjim@gmail.com> wrote:

> Thanks Natanael and Timo for your comments. I'm glad we're mostly on
> the same page about these things. Much of what I threw out was just to
> invite reactions.
> 
> I'll have a look at what you've already committed and rebase the
> commits Timo asked me to separate out, shortly. This is just to
> respond to some of your comments.
> 
> I'll split out the discussion of -R/-D and the logging to separate
> emails.
> 
> 
> On Fri, Jun 28, 2013 at 11:13:33AM +0200, Natanael Copa wrote:
> > >  static struct apk_applet apk_add = {
> > >  	.name = "add",
> > > -	.help = "Add/update PACKAGEs to main dependencies and
> > > install them",
> > > +	.help = "Add PACKAGEs to main dependencies and install
> > > (or upgrade) "
> > > +		"them, while ensuring that all dependencies are
> > > met", .arguments = "PACKAGE...",
> > >  	.open_flags = APK_OPENF_WRITE,
> > >  	.context_size = sizeof(struct add_ctx),
> > 
> > it could have been: (or upgrade, if -u is given)
> 
> How about this comment message for "apk add": "Register package in
> 'world' and install it, if necessary, while ensuring that all
> dependencies are met"
> 
> Perhaps adding, either there or on the help for the "-u" flag,
> something like "If the package is already installed, `apk add`
> ignores any updates that may be available, unless the `-u` option is
> provided"

Mmm... right. I originally wanted to have "apk --help" be nice looking
on 80 char wide displayes, and tried to keep them as small as possible.
Especially the applet descriptions.

I wonder if it would make sense to have short and long descriptions.

We were talking with ncopa that we really should auto-generate manpages
from the apk --help (either by doing a script to extract these message;
or at build time have extra applet 'asciidoc' to generate it).

> On Fri, Jun 28, 2013 at 11:19:03AM +0200, Natanael Copa wrote:
> > > @@ -183,8 +183,8 @@ static int ver_main(void *pctx, struct
> > > apk_database *db, struct apk_string_array 
> > >  static struct apk_option ver_options[] = {
> > >  	{ 'I', "indexes",	"Print description and versions
> > > of indexes" },
> > > -	{ 't', "test",		"Compare two given
> > > versions" },
> > > -	{ 'c', "check", 	"Check if the given version
> > > string is valid" },
> > > +	{ 't', "test",		"Compare two given
> > > versions, output '<' '=' or '>'" },
> > > +	{ 'c', "check", 	"Check the given version
> > > strings, output any that are invalid" }, { 'a',
> > > "all",		"Consider packages from all repository
> > > tags" }, { 'l', "limit",		"Limit output to packages
> > > with status matching one of LIMCHARs", required_argument,
> > > "LIMCHARs" },
> > 
> > shouldn't it be a comma (,) in there?
> > 
> > ...output '<', '=' or '>'
> 
> If you like. I'll have a look to see whether this is a commit you want
> me to re-supply.

I pushed this commit with the comma added.

> > > @@ -192,7 +192,8 @@ static struct apk_option ver_options[] = {
> > >  
> > >  static struct apk_applet apk_ver = {
> > >  	.name = "version",
> > > -	.help = "Compare package versions",
> > > +	.help = "Compare package versions (in installed database
> > > vs. available) "
> > > +		"or do tests on literal version strings (with or
> > > without -rN suffixes)", .open_flags = APK_OPENF_READ,
> > >  	.context_size = sizeof(struct ver_ctx),
> > >  	.num_options = ARRAY_SIZE(ver_options),
> > 
> > Can we drop the "(with or without -rN suffixes)" part? it looks
> > like -r -N options...
> 
> If you like. Is there some other way we might communicate the same
> thing? Otherwise there's no way for someone to figure this out except
> by experimenting. Which isn't sooo bad.

This is also pushed with the modification done.

> Right, the version I was originally developing these patches against
> would always give the warning, that's why I reworded it. I hadn't
> noticed that the logic had now changed.
> 
> I like the idea that Natanael and Timo were discussing of just using
> installed dependencies when self-upgrading---if the ABIs permit.

Ok. Since everyone seems to like it, I'll that then.

> > I think it should do this automatically. But I really don't like
> > duplicating the code how to handle each option in all the applets.
> > 
> > I do like the idea of pushing these options down the applets that
> > actually use them. So seems we need some sort of "option group"
> > support.
> 
> I understand where you're coming from. Pragmatically, though, the
> option-handling code is just a single bit-setting in each case.
> Replacing this with a duplicate bit-setting to indicate the "option
> group" doesn't seem to gain much.
> 
> On the other hand, maybe there are broader benefits to introducing
> option groups. Also, it's true that there is the duplicated usage
> message.

More importantly, if I code a new feature that belongs in the same
"option group", I don't want to add it in 4 applets. Or if I update the
description, I don't want to change it in 4 places. So yes, this is
definitely needed; even if it is only static inline function parsing
the options in default: block, and #define providing the options for
the applet structure.

But I'd rather make it more formal option group - so the man pages
could note that appropriately. And document each such group only once.
Instead of duplicating the information for each applet.

> On Fri, Jun 28, 2013 at 03:46:31PM +0200, Natanael Copa wrote:
> > apk pull reminds me of git pull. (which will do both fetch and
> > merge), but is an option yes.
> > 
> > other options:
> >   apk (re)fresh
> >   apk fetchdb
> >   apk syncdb
> > 
> > I'm not convinced it is worth it though.
> > 
> > maybe a single 'apk -U'?
> 
> Right, "git pull" was the model for my suggestion. "git fetch" is the
> closer analogy, but "apk fetch" is already used.
> 
> I like the idea of honoring bare "apk -U/--update-cache" the best. I
> think implementing that would require somewhat more ambitious code
> changes.

I quite like update/upgrade. But I do understand that it can be
problematic for some. We could override "apk fetch" without arguments
to get the indexes, but not sure if that is ideal either. Of the above
candidates I prefer "apk -U". It might make sense to change the long
name to plain --update, or --update-index (instead of --update-cache;
which is legacy thing when "cache" contained only indexes and not
packages. there's the separate "cache" applet now to manage the real
"cache").

> On Fri, Jun 28, 2013 at 01:21:45PM +0300, Timo Teras wrote:
> > On Thu, 27 Jun 2013 15:28:45 -0400
> > Dubiousjim <dubiousjim@gmail.com> wrote:
> > 
> > > * It'd be nice if "apk version" also displayed which repository an
> > > upgrade would come from.
> > 
> > It shows the pinning. For the exact repositories, one can now use
> > "apk policy <pkgname>". Do note that "apk version" says the latest
> > version, not the version it would upgrade to.
> 
> Wow, I didn't realize that at all.
>
> > Apk version is a wee bet overloaded, as it can do many unrelated
> > things. Might be time to split it.
> 
> I agree. Should some of this functionality be merged with the new
> policy applet? Haven't yet sorted that out in my mental map.

I was thinking to separate:
 version -> upgrade --analyze (and make it calculate the real upgrade)
   (for old style output: upgrade --analyze --latest)
 version -t -> new verstr applet
 version -c -> new verstr applet
 version -I -> index --version, policy --index, or ideas?

And nuke version applet completely.

> On Fri, Jun 28, 2013 at 01:21:45PM +0300, Timo Teras wrote:
> > On Thu, 27 Jun 2013 15:28:45 -0400
> > Dubiousjim <dubiousjim@gmail.com> wrote:
> > 
> > > * Is "--overlay-from-stdin" really a generic option, or like
> > > --clean-protected and --purge does this option too only apply to
> > > some of the applets?
> > 
> > It applies like --clean-protected. Applets that commit things to
> > database use it. In practice it is only used on tmpfs root setups
> > during initramfs bootstrap.
> 
> Ok, then should it also be pushed down into the applets that use it?
> Or the appropriate "option group"?

Correct. Should go to same "option group" with all these. Basically the
option group would be for parameters that affect committing changes to
repository.

Though, the overlay-from-stdin option is quite special. I'm actually
thinking if we should change the bootstrap code to have it's own
applet. And implement --initdb and --overlay-from-stdin only in that.

> On Fri, Jun 28, 2013 at 02:36:45PM +0200, Natanael Copa wrote:
> > On Thu, 27 Jun 2013 15:28:45 -0400
> > Dubiousjim <dubiousjim@gmail.com> wrote:
> >  
> > > 1. One set of changes is that we move some options that are now
> > > listed in the "generic options" usage block, but which don't in
> > > fact apply to all applets, into the applet-specific option set
> > > for the applets that use them. This applies to -U/--update-cache,
> > > --purge, and --clean-protected.
> > 
> > I think this is good. I am not 100% sure but I think that the short
> > form id integer might need to be universally unique.
> 
> Is that right? They can't even be duplicated between applets? I think
> there is some such duplication already in the code base. But this is
> just going by distant memory.

Duplicates between applets is ok. Uniqueness is needed with respect to
global options and per-applet options. If doing "option groups" then
that would need to be added to list.

Currently in case of duplicates, the global flags take precedence.

> On Fri, Jun 28, 2013 at 03:46:31PM +0200, Natanael Copa wrote:
> > 
> > apk fetch -s is useful to list contents of an uninstalled package:
> >   apk fetch --stdout pkg | tar -zt
> > 
> > ...
> > 
> > maybe change 'apk fetch -s' to 'apk fetch -O' similar to 'tar
> > -O' (and 'wget -O -')
> 
> I wasn't suggesting to eliminate the functionality, just the
> short-form option name "-s". I like the "-O" short-form much better.

Yes, -O <target> sounds good.

> On Fri, Jun 28, 2013 at 02:36:45PM +0200, Natanael Copa wrote:
> > On Thu, 27 Jun 2013 15:28:45 -0400
> > Dubiousjim <dubiousjim@gmail.com> wrote:
> >
> > > For the most part, these UI tweaks don't introduce anything that
> > > will break existing behavior---except in 1 place (or perhaps
> > > 1-and-a-half places), which I'll describe below...
> > 
> > I am a bit sceptic to this kind of changes. It basically means that
> > there are some things that simply cannot be used in scripts...
> 
> I understand. I was trying very hard to minimize the breakage. The
> -R/-D stuff needs more discussion. The "-and-a-half" thing I was
> referring to above was about "apk info -i", but that seems to be
> currently broken anyway (also the long form).
> 
> I was thinking we should make sure the public scripts use long-form
> options (I've verified that many of them already do, except for a
> bunch of uses of -q). I was thinking also that where possible,
> changes be introduced like this:
> 
>         * first the new applet name/option name is enabled, and the
>           old names are still honored, but the usage message suggests
> to use the new forms.
>         * a bit later, the old forms are no longer listed in the usage
>           message, but they are still honored, except that we also
>           at the same time emit a message to stderr.
> 
> This won't break anything that doesn't capture stderr.
> 
> Also when I suggest "eliminating" short option-names, I just
> mean easing them out of the recommended usage and so
> on, as above, but---where possible---still honoring them.
> 
> It's just in a few places that things really invite being cleaned up
> in a way that requires more breakage. The "apk info -R" is the
> dominant example. I wholly agree this needs more discussion and
> thought.
> 
> > ... unless we
> > provide a simple way for a script to fish out the apk scripting
> > "api" version.
> > 
> > Maybe introduce apk --api-version
> > 
> > if empty, then its current api, then whenever we change the
> > scripting 'api' or output format, we bump the "api-version" number.
> > 
> > That way scripts has a way to deal with this kind of changes in
> > future. like:
> > 
> > apk_api="$(apk --api-version 2>/dev/null)"
> > : ${apk_api:=0}
> > 
> > if [ $apk_api -le 1 ]; then
> > 	apk_mkindex="apk index";;
> > else
> > 	apk_mkindex="apk mkindex";;
> > fi
> > 
> > $apk_mkindex *.apk .... etc
> 
> That's heavy-handed. Maybe it might be called for in the end, but I
> hope we can find some lighter-weight and elegant solution. What I
> described above wouldn't introduce any breakage with respect to "apk
> index"; we'd just eventually not be documenting its existence, but
> would continue to honor it.
> 
> 
> On Thu, 27 Jun 2013 15:28:45 -0400
> Dubiousjim <dubiousjim@gmail.com> wrote:
> 
> > * "apk info" and "apk search" have different output styles, and
> > interact with "-q" and "-v" differently. Here is the current
> > behavior:
> > 
> >     apk info:     list all installed packages, without -1.0-r0
> > suffixes apk info -v:  list all installed packages, with -1.0-r0
> > suffixes apk info -vv: list all installed packages, with -1.0-r0
> > suffixes, also list descriptions
> >     apk info -v [moreoptions] packagename: machine-friendly output
> > 
> >     apk search substring:    list matching packages, with 1.0-r0
> > suffixes apk search -q substring: suppress -1.0-r0 suffixes
> >     apk search -v substring: list matching packages plus
> > descriptions, on one line
> > 
> >   I observe also that "apk info" always seems to ignore "-q", and
> > "apk version -q" is, perversely, much more verbose than "apk
> > version". 
> >   This is all confusing and hard to remember. It'd be nice if we
> > could standardize on a single mapping between output formats and
> > options, perhaps following the pattern of "apk search".
> 
> 
> On Fri, Jun 28, 2013 at 01:21:45PM +0300, Timo Teras wrote:
> > 
> > It might be also possible now to merge 'search' and 'info' applets
> > together. The fundamental difference is that info works with
> > installed packages, and search works with available packages - thus
> > info does have some extra information output that is available only
> > with installed packages. But all in all, most of the functionality
> > is shared.
> 
> Maybe. The main conflicts I can think of are with bare "apk info" vs
> bare "apk search", and with "apk info --rdepends" vs "apk search
> --rdepends". Also "apk info ..." requires an exact package name,
> right? whereas "apk search ..." defaults to accepting a glob pattern.
> 
> I don't mind having the separate applets.

True. With some thought, these could be merged to one .c file even if
we expose two separate applets. I would definitely want unified output
format for these.

Possibly also implement --json or similar for machine parseable output.

- Timo


---
Unsubscribe:  alpine-devel+unsubscribe@lists.alpinelinux.org
Help:         alpine-devel+help@lists.alpinelinux.org
---