On Nov 16, 2007 6:10 PM, Xavier <shiningxc@gmail.com> wrote:

What's even more strange is that the public alpm_db_whatprovides is defined in db.c . So any objections for eliminating provide.{c,h} , and moving _alpm_db_whatprovides to db.c ?

No qualms from me - housekeeping is always good

> c) Could you please put SOME debugging back. It was there for a
> reason, and removing it outright is bad idea.
> 
> Thanks,
> Aaron
1. To summarize my opinion about pacman debugging, please give a look at
smoke001.py output with --debug...
2. the old debugging was _fortunately_ broken (or at least misleading) in
alpm_depcmp: it printed debug message (match / no match) iff pkgname/provision
matched
3. alpm_depcmp is quite an atomic function, and can _prove_ easily if it works
correctly or not (for example alpm_list_next doesn't print debug information
neither) <- the only information in its debug message: its input parameters (but
see also 4.)
4. if you "fix" 2., then even a single-package transcation will lead to
_thousands_ of depcmp lines in debug (search for satisfier in local and sync
db-s); back to smoke001.py: the fix would add at least 1 million (!) depcmp
lines (alpm_graph_init)

Do you still want it?
If yes, with 2-fix or without it?
Bye


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/

Not directly related to your question, but are you taking care of all suggestions made to your patch? Because I started working on it too :) I was going to resubmit it there, so you can look at it and blame me for the bad things I did to your patch :)

Since I woke up ~1 hour ago, I couldn't take care of the suggestions (I answered to your questions immedietly after reading). So we didn't make parallel work ;-) Well, thx for your help. Anyway, to tell the truth I don't like implement things which I don't agree with (e.g. depcmp logging). So bigthx again. Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/

On Sat, Nov 17, 2007 at 02:06:11PM +0100, Nagy Gabor wrote:

...

b) Could you also add some documentation about this in the man page (especially that the format is name-space-version)

Well, I could add it (and now I really don't want to be lazy), but my English is... you know.

I'm done now, I think I took care of all the comments/suggestions made. So I added a little comment in PKGBUILD man page as well. See chantry dot homelinux dot org/~xav/gitweb/gitweb.cgi?p=pacman.git;a=shortlog;h=working Sorry for not providing the url, but I did once, and noticed some bots are looking for it now, and that annoys me :D

Hmm. There are some interesting branches here... Especially the 'sync' one ;-) Now I'm going offline for this weekend, but I will vet you (and blame of course ;-P). Bye, ngaba

I forgot to say 'thank you': thx Bye ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/

On Sat, Nov 17, 2007 at 05:55:44PM +0100, Nagy Gabor wrote:

I'm writing from an internet caffee, because the results I found shocked me;-) The blames (just joking): 1. "... 'cron 2.0' to satisfy the 'cron>=2.0' dependency of other packages.". Only valid version numbers are allowed, for example 2.0-1.

I actually hesitated when writing this. And I'm still confused. Is it ok to have a 'cron>=2.0' dependency, but not providing 'cron 2.0' ? Most versioned dependencies don't include any revisions. See grep -r 'depends.*>=' /var/abs/core

2. I found a hidden alpm_dep_get_string _alpm_checkdeps patch in your patch: Avoiding alpm_dep_get_string usage in debug messages was purposeful by me, not negligency (I refer to http://projects.archlinux.org/git/? p=pacman.git;a=commit;h=d903fc607ee2aa2527202f1e54a44be325eabe48 here) I don't want to waste time for formatting debug messages (I prefer only O(1) debug preformat), see also: 3.

I indeed thought this was a negligency. It didn't seem to work for me. For example when running the add045 pactest, I got messages like : [19:17:19] debug: missing dependency '' for package 'pkg1' I switched to alpm_dep_get_string usage and it worked fine.

3. I simply didn't understand why was alpm_splitdep so inefficient. The only thing I could imagine the horrible number of malloc/free calls: http://www.archlinux.org/pipermail/pacman-dev/2007-November/010027.html alpm_depcmp is also an atomic function: smoke001.py calls it at least one million (!) times. That's why I tried to avoid free/malloc usage... (your patch duplicates the string instead of my ugly hack + preformat alpm_depcmp debug with alpm_dep_get_string). The same for _alpm_provision_cmp.

Yes, I saw that and I answered : http://www.archlinux.org/pipermail/pacman-dev/2007-November/010033.html For a more realistic usage like -Qt, I couldn't measure any differences. It'll probably be easier to see a difference with smoke001, but is that really problematic? I didn't like that const strings were modified in _alpm_provision_cmp, so I duplicated the strings. And since the code im depcmp was similar, I did the same there, to keep things consistent. And I put a debug back because Aaron asked for it, and I also think it can be very handy sometimes, to debug quicker.

So I did some speed test, and the results was really suprising: First of all, I kept only deps.c.diff from our patches (my typo doesn't matter here). And I did "pactest.py -v" on smoke001.py (empty disk cache, repeated many times times etc.) The result: your patch is _significantly_ slower (I got 5,4,4,4, ... sec) than mine (I got 4,3,3,3, ... sec). I repeated the test 4 times (patch pacman, compile, 5 pactest); the result is always the same! Could you test this, too?

Hm yes, I'll do more tests.

On Nov 19, 2007 8:44 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:

...

2. There is something in my mind about alpm_dep_getstring: It shouldn't be used for debug preformat (or in libalpm at all); but that is good for front-ends. I would prefer the following more elegant(?) solution instead of switch(): there is a global const char *deptypestr[] = {">=", "<=", "="} in the correct order somewhere; and from now on, you can pass deptypestr[dep->mod] parameter easily to alpm_log for example (see alpm_depcmp). This is O(1) (<- switch is also O(1), but that floods the code imho) I will create a patch for this, after I can sync my git repo ;-) Bye

Hmmm. I think I actually like the alpm_dep_get_string function. I don't like the need to free the string afterwards, as it adds more mental book keeping into the mix. I'm a little torn here, as this patch is much less book keeping, but is a little... erm ,uglier. Anyone else have an opinion one way or the other?

On Mon, Nov 19, 2007 at 03:44:08PM +0100, Nagy Gabor wrote:

...

2. There is something in my mind about alpm_dep_getstring: It shouldn't be used for debug preformat (or in libalpm at all); but that is good for front-ends. I would prefer the following more elegant(?) solution instead of switch(): there is a global const char *deptypestr[] = {">=", "<=", "="} in the correct order somewhere; and from now on, you can pass deptypestr[dep->mod] parameter easily to alpm_log for example (see alpm_depcmp). This is O(1) (<- switch is also O(1), but that floods the code imho) I will create a patch for this, after I can sync my git repo ;-) Bye

I would say this is a neat hack :) neat because it gives the same result in much less lines. hack because : 1) it depends on the actual values of PM_DEP_MOD_* 2) Since the PM_DEP_MOD_* starts at 1, you need to use an empty first column, which I found quite funny :)

+const char *pmdepmodstr[] = {"", "", "=", ">=", "<="};

+ _alpm_log(PM_LOG_DEBUG, "%s satisfies dependency %s%s%s -- skipping\n", + alpm_pkg_get_name(sp), missdep->name, pmdepmodstr[missdep->mod], missdep->version);

And finally, that's really a PITA just to print a dependency. Though we don't need to print dependencies everywhere, but well, I don't know.. I don't think the current situation is that bad.

was sent j->data stored the dependency string, now it stores pmdeptype_t pmdepend_t

---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/

On Nov 19, 2007 5:11 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:

...

Yes, I saw that and I answered : http://www.archlinux.org/pipermail/pacman-dev/2007-November/010033.html

For a more realistic usage like -Qt, I couldn't measure any differences. It'll probably be easier to see a difference with smoke001, but is that really problematic?

I didn't like that const strings were modified in _alpm_provision_cmp, so I duplicated the strings. And since the code im depcmp was similar, I did the same there, to keep things consistent.

And I put a debug back because Aaron asked for it, and I also think it can be very handy sometimes, to debug quicker.

IMHO pacman debugging tends to be unusable. The debug output of smoke001.py is now ~100MB(!!); before alpm_depcmp logging it was "only" ~30MB. alpm_depcmp debugging added 1.5 million (!) lines. So even the compressed output is 5 MB for me (user cannot attach this easily with an e-mail etc. etc.). And don't forget that alpm_depcmp is used everywhere, so this is not an extreme example: compute_requiredby, depcheck etc. I simply cannot understand how this can help us. The debug.log is totally unreadable now (at least to me), you have to know the debug format and search for the interesting part (by doing this you may miss something...). Logging atomic functions is simply needless imho (the same for "checking target1 target2 conflict"-like messages); the caller function usually informs us about the result in a debug message (foo satisfies dependency bar, pulling into the target list; foo conflicts with installed package bar, bar selected for removal ...). Bye, ngaba

Well its better that you brought it up on the list first, so I do appreciate that. And if you submit a patch to do *just this*, it is much more likely to get considered than if you try to lump this change together in another patch. I do agree that debugging is less useful when the function is stateless, so that would be one reason to get rid of this debug logging in depcmp. Next, I will say that depcmp is a fairly stable function (although we did just change it)- I have faith that it will work. Finally, the debug text does carry some extra CPU cycles with the alpm_dep_get_string() call, so that would be justification to remove as well. Of course, you made it sound like such a big hassle to look through debug logs. Wouldn't something along the lines of "sed -i '/depcmp/d' <logfile>" work just fine? So in short- submit a patch for *one* feature at a time, and justfiy it in the commit message. Don't try to sneak things by. I do think this may be a valid complaint though. -Dan

On Nov 19, 2007 6:55 AM, Dan McGee <dpmcgee@gmail.com> wrote:

...

So in short- submit a patch for *one* feature at a time, and justfiy it in the commit message. Don't try to sneak things by. I do think this may be a valid complaint though.

Even better? How about something similar to:

#ifdef PACMAN_DEBUG # define DEBUG_LOG(...) _alpm_log(PM_LOG_DEBUG, __VA_ARGS__) #else # define DEBUG_LOG(x) #endif

Then just: s/_alpm_log(PM_LOG_DEBUG,/DEBUG_LOG(/g

This way we could simply get rid of all debugging if we wanted to. Then you have both a useful version for debugging, and a "snappy" version that saves 1 or 2 seconds without having to output anything.

I like this idea, but: What about user feedbacks? We will ask them to download a debugging-enabled version of pacman? Bye, ngaba ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/

> On Sat, Nov 17, 2007 at 05:55:44PM +0100, Nagy Gabor wrote:
> > I'm writing from an internet caffee, because the results I found shocked
> me;-)
> > The blames (just joking):
> > 1. "... 'cron 2.0' to satisfy the 'cron>=2.0' dependency of other
> packages.".
> > Only valid version numbers are allowed, for example 2.0-1.
> 
> I actually hesitated when writing this. And I'm still confused.
> Is it ok to have a 'cron>=2.0' dependency, but not providing 'cron 2.0' ?
> Most versioned dependencies don't include any revisions.
> See grep -r 'depends.*>=' /var/abs/core
1. As I told in the subject: conflict-version is undocumented (what's more, the
documentation says you cannot use version numbers...). So we should fix this:
a.) conflict syntax is the same as depends syntax
b.) 'foo' conflict covers exactly the same set of packages as 'foo' depends (so
for example (versioned ;-) provisions are also checked).

2. Back to versioned provisions:
My concept of versioned provision was: alternative (pkgname, pkgver) pairs
[which means this package is equivalent with _one_ certain package], where
pkgver="" is also allowed [which means in my concept that pkgver was "lost", NOT
any; this decision is crutial here, one of the must important changes(*)]<- this
is the clearest and easiest-to-implement solution imho (you needn't modify
alpm_vercmp).
So for example kernelck-2.6.22-1 provides kernel-2.6.22-1 and so on.

Vmiklos suggested on irc, that I should use depends-like syntax; such as
provides 'foo>=1.0' but I didn't like that because this can lead to decision
problems. What do you think? If you also prefer this (Xavier, you mentioned
dependencies here, too), then my compromise: change the 'provname provver'
syntax to 'provname=provver' (this is just a s/' '/'='/ in my patch), and later
you can implement more general provides syntax without breaking the current "style".

(*)So I ask you again (you should make a decision):
What should versionless provision mean?
a.) this provision does NOT satisfy 'provision>=2.0-1' (I've chosen this in my
patch)
b.) 'no provver' means 'ANY provver' (this is also acceptable, but imho
'provision>=2.0-1' means that we cannot accept "any" provision, just the "new" ones)
c.) current method: use pkgver (imho forget it ;-)

Bye, ngaba


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/

...

1. As I told in the subject: conflict-version is undocumented (what's more,

...

documentation says you cannot use version numbers...). So we should fix

On Mon, Nov 19, 2007 at 01:16:10PM +0100, Nagy Gabor wrote: the this:

...

a.) conflict syntax is the same as depends syntax b.) 'foo' conflict covers exactly the same set of packages as 'foo' depends (so for example (versioned ;-) provisions are also checked).

Attaching a little patch.

Looks fine. Thanks.

...

2. Back to versioned provisions: My concept of versioned provision was: alternative (pkgname, pkgver) pairs [which means this package is equivalent with _one_ certain package], where pkgver="" is also allowed [which means in my concept that pkgver was "lost", NOT any; this decision is crutial here, one of the must important changes(*)]<- this is the clearest and easiest-to-implement solution imho (you needn't modify alpm_vercmp). So for example kernelck-2.6.22-1 provides kernel-2.6.22-1 and so on.

Vmiklos suggested on irc, that I should use depends-like syntax; such as provides 'foo>=1.0' but I didn't like that because this can lead to decision problems. What do you think? If you also prefer this (Xavier, you mentioned dependencies here, too), then my compromise: change the 'provname provver' syntax to 'provname=provver' (this is just a s/' '/'='/ in my patch), and later you can implement more general provides syntax without breaking the current "style".

(*)So I ask you again (you should make a decision): What should versionless provision mean? a.) this provision does NOT satisfy 'provision>=2.0-1' (I've chosen this in my patch) b.) 'no provver' means 'ANY provver' (this is also acceptable, but imho 'provision>=2.0-1' means that we cannot accept "any" provision, just the "new" ones) c.) current method: use pkgver (imho forget it ;-)

I found the current behavior a little confusing at first, but actually, it's alright. Suppose you have a package which depends on provision>=2.0-2 , and let's see what a package needs to provide for satisfying it : * 'provision' -> NO * 'provision 2.0' -> OK * 'provision 2.0-1' -> NO * 'provision 2.0-2' -> OK

So any 2.0-x dep will be satisfied by provision 2.0 .

Now, if the dependency is provision>=2.0 : * 'provision' -> NO * 'provision 2.0' -> OK * 'provision 2.0-1' -> OK * 'provision 2.0-2' -> OK

So the 2.0 dep will be satified by any 2.0-x

Well. Then choice b.) would be more plausible. But I can repeat myself: If a devel adds 'provision>=2.0-2' as depend, then this implicitly means that not "all" 'provision 2.0-x' version would satisfy this depend; and 'provision 2.0' says no info about -x... Personally I trusted on alpm_versioncmp [he decides if 2.0>=2.0-2 or not: currently it says yes, I say no], that's why I defined provversion as _valid_ package version. This provversion definition looks strict first. But if you think over this is not strict at all: You want to write "provision 2.0"; but I ask you to write "provision 2.0-1". What will happen? "provision 2.0-1" will satisfy 'provision>=2.0' but won't satisfy 'provision>=2.1' and so on. IMHO -x is rarely used in depends, so your -1 is doesn't count at all. If the package needs 'provision>=2.0-2' (because 'provision 2.0-1' is "buggy" <- rare case, because the source-code of 2.0-2 and 2.0-1 is the same), then we can easily decide whether your provision satisfies this or not. So we didn't lose anything, but we won can-decide-always. My reasoning above followed the the reasoning of a.) One may prefer b.), when provision defines a _set_ of "packages": so 'provision 2.0' means 'provision 2.0-ANY' <- this is how alpm_versioncmp works now. My main contra: In this case provision 'nvidia' would satisfy all 'nvidia ...' depend. So the devel simply cannot force pacman to upgrade the outdated dependency package which provides nvidia (which was installed from aur, -Su doesn't update it for example), but in fact this old nvidia provider is not sufficient for "nvidia>=123.456-7" depend. If the devel keeps this in mind, then he probably never uses versionless provision (now you can find versionless provisions only in the dbs), which is a big restriction. Bye, ngaba ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/

On Nov 19, 2007 12:42 PM, Scott Horowitz <stonecrest@gmail.com> wrote:

...

...

Suppose you have a package which depends on provision>=2.0-2 * 'provision 2.0' -> OK

If a devel adds 'provision>=2.0-2' as depend, then this implicitly means that not "all" 'provision 2.0-x' version would satisfy this depend; and 'provision 2.0' says no info about -x...

I agree with Nagy; I think just about everyone would find 2.0 meeting a >=2.0-2 provision to be illogical.

I'm pretty sure normal deps already act this way. That is, the vercmp function will check the pkgrel only if it exists. If it doesn't exist, it doesn't care. This gives us flexibility. For instance, lets say we have 19 packages that depend on foobar 1.7. If we add "foobar=1.7-1" to every package, that's fine. Later, if we change, say the url=() in the PKGBUILD, we now need to rebuild all of the packages that depend on foobar with explicit versions. It's just messy. Instead of forcing it one way or the other, leaving it open like the current vercmp does gives the power of this decision to the packager.

On Mon, Nov 19, 2007 at 07:28:01PM +0100, Nagy Gabor wrote:

If a devel adds 'provision>=2.0-2' as depend, then this implicitly means that not "all" 'provision 2.0-x' version would satisfy this depend; and 'provision 2.0' says no info about -x...

Versioned dependencies with a release are a minority. So it's not a big deal. If you still aren't satisfied, could we please talk about real cases where this kind of dependency is used?

On Nov 19, 2007 11:55 AM, Xavier <shiningxc@gmail.com> wrote:

I found the current behavior a little confusing at first, but actually, it's alright. Suppose you have a package which depends on provision>=2.0-2 , and let's see what a package needs to provide for satisfying it : * 'provision' -> NO * 'provision 2.0' -> OK * 'provision 2.0-1' -> NO * 'provision 2.0-2' -> OK

So any 2.0-x dep will be satisfied by provision 2.0 .

Now, if the dependency is provision>=2.0 : * 'provision' -> NO * 'provision 2.0' -> OK * 'provision 2.0-1' -> OK * 'provision 2.0-2' -> OK

So the 2.0 dep will be satified by any 2.0-x

OK, there have been many replies, but all I want to say is this- this is *good enough* for me. We are talking niche cases here, we have much bigger problems in the code to solve than what basically boils down to a feature request in a brand new feature. -Dan

On Fri, Nov 16, 2007 at 06:12:28PM -0600, Aaron Griffin wrote:

Just a few comments: a) Awesome. It's great that you did this b) Could you also add some documentation about this in the man page (especially that the format is name-space-version) c) Could you please put SOME debugging back. It was there for a reason, and removing it outright is bad idea.

That implementation from Nagy is quite different from the one you had in mind (FS#6508). It was not made at the same level. Any drawbacks in the way Nagy did it?

On Sat, Nov 17, 2007 at 12:42:51PM +0100, Nagy Gabor wrote:

...

And I think git add is only necessary for the untracked files : that is, new files that don't exist yet in the git repo, like new pactests. Well, then you are not familiar in git neither ;-) If I modify something, without git-add then git commit -s doesn't won't do anything. First you must tell git about the modified files (git-add -u for example). The thing I dunno, if this is needed more than once or not (I mean, what happens if I modify files after git-add) Bye

Oh, what I said might only apply to : git-commit -a -s . That should automatically add the tracked files that were modified. Hm yes, just checked the manpage :) -a|--all Tell the command to automatically stage files that have been modified and deleted, but new files you have not told git about are not affected.

Hm, why did you checkout -f ? Did you want to switch to another branch? If so, you always try with checkout first. and if it isn't happy because of local changes, then you carefully check what these local changes are. Thanks, I will keep this. And as I can recall from my memory hopefully this patch is not far from the last state [/me is not so disappointed now] Bye

---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/