[pacman-dev] libalpm data structures

newer
Re: [pacman-dev] [arch-dev-public]...

older
[pacman-dev] [PATCH] Add support...

Nagy Gabor

19 Oct 2007 19 Oct '07

4:56 p.m.

Hi! This is discussion-only (==no-patch) e-mail, mainly about code clean-up, so if you don't like that type of e-mails, please skip this. Xavier's compute_requiredby catch was the motivation of this e-mail: I never liked the following method: pmdepend_t *dep = alpm_splitdep(depend) /* char -> pmdepend_t conversion */ ...alpm_depcmp(pkg, dep)... FREE(dep) /* usually forgotten */ So I asked myself: Do we really need pmdepend_t struct? Why don't we use it in alpm_depcmp only ("internally")? So the depend-input of alpm_depcmp could be char* (and pmdepend_t should be disappear from libalpm API). Then I read the .h files of libalpm. My impressions: 1. We don't need pmdepend_t => pmdepmod_t. See above. This is mainly used in pmdepmissing_t which is used to collect missing dependencies (pmdepmissing_t is not needed here neither), and to print error/log messages (usually pmdepend_t -> char conversion is done here, too); where we could also use char. 2. We have pmconflict_t. Nice. So we don't need pmdeptype_t (in pmdepmissing_t). As we discussed with Xavier, pmconflict_t should be a symmetric structure: {target,ctarget} set [<=> alpm_strcmp ordered (target, ctarget) pair] instead of (target,ctarget) ordered pair) <- this is a pmconflict_t-helper-function problem only. Future plans: 3. I still want to kill pmsyncpkg_t => pmsynctype_t (the ugliest data struct of libalpm) 4. If my preferred universal transaction becomes true, we won't need pmtranstype_t. And of course, if we rip off some unneeded structures, we also rip off tons of unneeded associated helper functions. Personally, I want to make a patch for 1. and 2. So if you don't like the idea, let me know. However, I'm a bit unsure in 2.: should I keep target and ctarget as char, or I should use pmpkg_t* instead <- faster but more "dangerous". Bye, ngaba ---------------------------------------------------- SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu This mail sent through IMP: http://horde.org/imp/

Show replies by date

Nagy Gabor

19 Oct 19 Oct

5 p.m.

...

pmdepmissing_t which is used to collect missing dependencies (pmdepmissing_t is not needed here neither), ... Oh, I wanted to say: (pmdepend_t is not needed here neither) Bye, ngaba

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

Aaron Griffin

11:14 p.m.

On 10/19/07, Nagy Gabor wrote:

...

Personally, I want to make a patch for 1. and 2. So if you don't like the idea, let me know. However, I'm a bit unsure in 2.: should I keep target and ctarget as char, or I should use pmpkg_t* instead <- faster but more "dangerous".

If I could make a suggestion: if it were up to me, I'd do 3. first, as it bugs me the most.

Nagy Gabor

20 Oct 20 Oct

10:30 a.m.

...

If I could make a suggestion: if it were up to me, I'd do 3. first, as it bugs me the most. But my preferred solution of 3. depends on my pending patches. So I'm waiting for you patiently ;-) And 3. needs some deeper changes (the code must be almost ready for the universal transaction, because I'd prefer upgrade and remove list instead of pmsyncpkg_t). And I'm a bit unsure: http://www.archlinux.org/pipermail/pacman-dev/2007-September/009448.html / "so the real question is: do we need this glue?" + 1. Bye

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

Xavier

27 Oct 27 Oct

2:57 p.m.

On Sat, Oct 20, 2007 at 12:30:27PM +0200, Nagy Gabor wrote:

...

...
If I could make a suggestion: if it were up to me, I'd do 3. first, as it bugs me the most. But my preferred solution of 3. depends on my pending patches. So I'm waiting for you patiently ;-) And 3. needs some deeper changes (the code must be almost ready for the universal transaction, because I'd prefer upgrade and remove list instead of pmsyncpkg_t). And I'm a bit unsure: http://www.archlinux.org/pipermail/pacman-dev/2007-September/009448.html / "so the real question is: do we need this glue?" + 1.

I don't really know about that last question, but since this would require deep changes, we indeed need to go step by step. So we first need to concentrate on getting your pending patches that go in this direction merged.

Nagy Gabor

21 Oct 21 Oct

7:33 p.m.

...

2. We have pmconflict_t. Nice. So we don't need pmdeptype_t (in pmdepmissing_t). As we discussed with Xavier, pmconflict_t should be a symmetric structure: {target,ctarget} set [<=> alpm_strcmp ordered (target, ctarget) pair] instead of (target,ctarget) ordered pair) <- this is a pmconflict_t-helper-function problem only. Future plans: Well, I overlooked something. pmconflict_t is reserved for file-conflicts only, and pmdepmissing_t is used for conflicts?! (UGLY) So here is a little patch which renames pmconflict_t to pmfileconflict_t and alpm_conflict_* to alpm_fileconflict_*. pmconflicttype_t was removed from pmfileconflict_t structure, because type can be determined with (ctarget == ""). Bye, ngaba

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 880bbeb..a40075a 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -52,7 +52,7 @@ typedef struct __pmtrans_t pmtrans_t; typedef struct __pmsyncpkg_t pmsyncpkg_t; typedef struct __pmdepend_t pmdepend_t; typedef struct __pmdepmissing_t pmdepmissing_t; -typedef struct __pmconflict_t pmconflict_t; +typedef struct __pmfileconflict_t pmfileconflict_t; typedef struct __pmgraph_t pmgraph_t; /* @@ -389,10 +389,10 @@ typedef enum _pmconflicttype_t { PM_CONFLICT_TYPE_FILE } pmconflicttype_t; -const char *alpm_conflict_get_target(pmconflict_t *conflict); -pmconflicttype_t alpm_conflict_get_type(pmconflict_t *conflict); -const char *alpm_conflict_get_file(pmconflict_t *conflict); -const char *alpm_conflict_get_ctarget(pmconflict_t *conflict); +const char *alpm_fileconflict_get_target(pmfileconflict_t *conflict); +pmconflicttype_t alpm_fileconflict_get_type(pmfileconflict_t *conflict); +const char *alpm_fileconflict_get_file(pmfileconflict_t *conflict); +const char *alpm_fileconflict_get_ctarget(pmfileconflict_t *conflict); /* * Helpers diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index d09c996..2aea8ff 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -230,22 +230,20 @@ static alpm_list_t *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB) return(ret); } -/* Adds pmconflict_t to a conflicts list. Pass the conflicts list, type (either +/* Adds pmfileconflict_t to a conflicts list. Pass the conflicts list, type (either * PM_CONFLICT_TYPE_TARGET or PM_CONFLICT_TYPE_FILE), a file string, and either * two package names or one package name and NULL. This is a wrapper for former * functionality that was done inline. */ -static alpm_list_t *add_fileconflict(alpm_list_t *conflicts, - pmconflicttype_t type, const char *filestr, +static alpm_list_t *add_fileconflict(alpm_list_t *conflicts, const char *filestr, const char* name1, const char* name2) { - pmconflict_t *conflict = malloc(sizeof(pmconflict_t)); + pmfileconflict_t *conflict = malloc(sizeof(pmfileconflict_t)); if(conflict == NULL) { _alpm_log(PM_LOG_ERROR, _("malloc failure: could not allocate %d bytes\n"), - sizeof(pmconflict_t)); + sizeof(pmfileconflict_t)); return(conflicts); } - conflict->type = type; strncpy(conflict->target, name1, PKG_NAME_LEN); strncpy(conflict->file, filestr, CONFLICT_FILE_LEN); if(name2) { @@ -303,8 +301,7 @@ alpm_list_t *_alpm_db_find_conflicts(pmdb_t *db, pmtrans_t *trans, char *root) if(tmpfiles) { for(k = tmpfiles; k; k = k->next) { snprintf(path, PATH_MAX, "%s%s", root, (char *)k->data); - conflicts = add_fileconflict(conflicts, PM_CONFLICT_TYPE_TARGET, path, - alpm_pkg_get_name(p1), alpm_pkg_get_name(p2)); + conflicts = add_fileconflict(conflicts, path, alpm_pkg_get_name(p1), alpm_pkg_get_name(p2)); } alpm_list_free_inner(tmpfiles, &free); alpm_list_free(tmpfiles); @@ -402,8 +399,7 @@ alpm_list_t *_alpm_db_find_conflicts(pmdb_t *db, pmtrans_t *trans, char *root) } if(!resolved_conflict) { _alpm_log(PM_LOG_DEBUG, "file found in conflict: %s\n", path); - conflicts = add_fileconflict(conflicts, PM_CONFLICT_TYPE_FILE, - path, p1->name, NULL); + conflicts = add_fileconflict(conflicts, path, p1->name, NULL); } } } @@ -414,7 +410,7 @@ alpm_list_t *_alpm_db_find_conflicts(pmdb_t *db, pmtrans_t *trans, char *root) return(conflicts); } -const char SYMEXPORT *alpm_conflict_get_target(pmconflict_t *conflict) +const char SYMEXPORT *alpm_fileconflict_get_target(pmfileconflict_t *conflict) { ALPM_LOG_FUNC; @@ -425,7 +421,7 @@ const char SYMEXPORT *alpm_conflict_get_target(pmconflict_t *conflict) return conflict->target; } -pmconflicttype_t SYMEXPORT alpm_conflict_get_type(pmconflict_t *conflict) +pmconflicttype_t SYMEXPORT alpm_fileconflict_get_type(pmfileconflict_t *conflict) { ALPM_LOG_FUNC; @@ -433,10 +429,10 @@ pmconflicttype_t SYMEXPORT alpm_conflict_get_type(pmconflict_t *conflict) ASSERT(handle != NULL, return(-1)); ASSERT(conflict != NULL, return(-1)); - return conflict->type; + if(conflict->ctarget[0] == '\0') return (PM_CONFLICT_TYPE_FILE); else return(PM_CONFLICT_TYPE_TARGET); } -const char SYMEXPORT *alpm_conflict_get_file(pmconflict_t *conflict) +const char SYMEXPORT *alpm_fileconflict_get_file(pmfileconflict_t *conflict) { ALPM_LOG_FUNC; @@ -447,7 +443,7 @@ const char SYMEXPORT *alpm_conflict_get_file(pmconflict_t *conflict) return conflict->file; } -const char SYMEXPORT *alpm_conflict_get_ctarget(pmconflict_t *conflict) +const char SYMEXPORT *alpm_fileconflict_get_ctarget(pmfileconflict_t *conflict) { ALPM_LOG_FUNC; diff --git a/lib/libalpm/conflict.h b/lib/libalpm/conflict.h index 8928de8..955fc38 100644 --- a/lib/libalpm/conflict.h +++ b/lib/libalpm/conflict.h @@ -27,9 +27,8 @@ #define CONFLICT_FILE_LEN 512 -struct __pmconflict_t { +struct __pmfileconflict_t { char target[PKG_NAME_LEN]; - pmconflicttype_t type; char file[CONFLICT_FILE_LEN]; char ctarget[PKG_NAME_LEN]; }; diff --git a/src/pacman/add.c b/src/pacman/add.c index 0b59a23..d430bf6 100644 --- a/src/pacman/add.c +++ b/src/pacman/add.c @@ -169,18 +169,18 @@ int pacman_add(alpm_list_t *targets) break; case PM_ERR_FILE_CONFLICTS: for(i = data; i; i = alpm_list_next(i)) { - pmconflict_t *conflict = alpm_list_getdata(i); - switch(alpm_conflict_get_type(conflict)) { + pmfileconflict_t *conflict = alpm_list_getdata(i); + switch(alpm_fileconflict_get_type(conflict)) { case PM_CONFLICT_TYPE_TARGET: printf(_("%s exists in both '%s' and '%s'\n"), - alpm_conflict_get_file(conflict), - alpm_conflict_get_target(conflict), - alpm_conflict_get_ctarget(conflict)); + alpm_fileconflict_get_file(conflict), + alpm_fileconflict_get_target(conflict), + alpm_fileconflict_get_ctarget(conflict)); break; case PM_CONFLICT_TYPE_FILE: printf(_("%s: %s exists in filesystem\n"), - alpm_conflict_get_target(conflict), - alpm_conflict_get_file(conflict)); + alpm_fileconflict_get_target(conflict), + alpm_fileconflict_get_file(conflict)); break; } } diff --git a/src/pacman/sync.c b/src/pacman/sync.c index bf6eed1..0feff97 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -632,18 +632,18 @@ int sync_trans(alpm_list_t *targets, int sync_only) alpm_list_t *i; case PM_ERR_FILE_CONFLICTS: for(i = data; i; i = alpm_list_next(i)) { - pmconflict_t *conflict = alpm_list_getdata(i); - switch(alpm_conflict_get_type(conflict)) { + pmfileconflict_t *conflict = alpm_list_getdata(i); + switch(alpm_fileconflict_get_type(conflict)) { case PM_CONFLICT_TYPE_TARGET: printf(_("%s exists in both '%s' and '%s'\n"), - alpm_conflict_get_file(conflict), - alpm_conflict_get_target(conflict), - alpm_conflict_get_ctarget(conflict)); + alpm_fileconflict_get_file(conflict), + alpm_fileconflict_get_target(conflict), + alpm_fileconflict_get_ctarget(conflict)); break; case PM_CONFLICT_TYPE_FILE: printf(_("%s: %s exists in filesystem\n"), - alpm_conflict_get_target(conflict), - alpm_conflict_get_file(conflict)); + alpm_fileconflict_get_target(conflict), + alpm_fileconflict_get_file(conflict)); break; } }

Xavier

23 Oct 23 Oct

11:21 p.m.

On Sun, Oct 21, 2007 at 09:33:42PM +0200, Nagy Gabor wrote:

...

Well, I overlooked something. pmconflict_t is reserved for file-conflicts only, and pmdepmissing_t is used for conflicts?! (UGLY) So here is a little patch which renames pmconflict_t to pmfileconflict_t and alpm_conflict_* to alpm_fileconflict_*.

I also found this confusing when I first saw it, so I think I like your change :)

...

pmconflicttype_t was removed from pmfileconflict_t structure, because type can be determined with (ctarget == "").

Well, I'm less sure about that, but I don't know really. What do others think?

Nagy Gabor

24 Oct 24 Oct

8:38 a.m.

...

...
pmconflicttype_t was removed from pmfileconflict_t structure, because type can be determined with (ctarget == "").

Well, I'm less sure about that, but I don't know really. What do others think?

That's why I "kept" alpm_fileconflict_get_type. However, pmconflicttype_t should be pmfileconflicttype_t, but I found that too long; and for example pmfileconflicttype_t == PM_CONFLICT_TYPE_FILE sounds silly; so I can accept your ideas for the new names. Bye, ngaba

Dan McGee

30 Oct 30 Oct

12:23 a.m.

On 10/23/07, Xavier wrote:

...

On Sun, Oct 21, 2007 at 09:33:42PM +0200, Nagy Gabor wrote:

...
Well, I overlooked something. pmconflict_t is reserved for file-conflicts only, and pmdepmissing_t is used for conflicts?! (UGLY) So here is a little patch which renames pmconflict_t to pmfileconflict_t and alpm_conflict_* to alpm_fileconflict_*.

I also found this confusing when I first saw it, so I think I like your change :)

...
pmconflicttype_t was removed from pmfileconflict_t structure, because type can be determined with (ctarget == "").

Well, I'm less sure about that, but I don't know really. What do others think?

I'm just confused overall here, and here is what I think should be done in some way or another. File conflicts and target (package) conflicts are completely different things, and should in now way be handled with the same struct. I don't know what a ctarget is- that is a terrible name. Let's get rid of it. I feel like we can simplify dep handling/target conflicts down to one logical group, and file conflicts down to another. The file conflicts stuff is easy; the package conflicts/depends is not so easy. Package conflicts: type packageconflict { Package 1 (name, version) Package 2 (name, version) (and some magic to handle depends, conflicts, and the current mod operator) } File conflicts: type fileconflict { String filename Location 1 (a package) Location 2 (a package OR the filesystem) } Thoughts? I think Nagy found some code here that definitely isn't up to snuff, we just need to make sure we fix it in the right way. -Dan

Nagy Gabor

10:02 a.m.

...

I'm just confused overall here, and here is what I think should be done in some way or another. File conflicts and target (package) conflicts are completely different things, and should in now way be handled with the same struct. Well, I will disappoint you :-P: target conflicts are handled in pmdepmiss_t now!!

...

I don't know what a ctarget is- that is a terrible name. Let's get rid of it. I suppose, ctarget means "conflicting target"

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

Xavier

31 Oct 31 Oct

12:42 p.m.

On Mon, Oct 29, 2007 at 07:23:25PM -0500, Dan McGee wrote:

...

On 10/23/07, Xavier wrote:

...
On Sun, Oct 21, 2007 at 09:33:42PM +0200, Nagy Gabor wrote:

...
Well, I overlooked something. pmconflict_t is reserved for file-conflicts only, and pmdepmissing_t is used for conflicts?! (UGLY) So here is a little patch which renames pmconflict_t to pmfileconflict_t and alpm_conflict_* to alpm_fileconflict_*.

I also found this confusing when I first saw it, so I think I like your change :)

...
pmconflicttype_t was removed from pmfileconflict_t structure, because type can be determined with (ctarget == "").

Well, I'm less sure about that, but I don't know really. What do others think?

I'm just confused overall here, and here is what I think should be done in some way or another. File conflicts and target (package) conflicts are completely different things, and should in now way be handled with the same struct.

I don't know what a ctarget is- that is a terrible name. Let's get rid of it.

I feel like we can simplify dep handling/target conflicts down to one logical group, and file conflicts down to another. The file conflicts stuff is easy; the package conflicts/depends is not so easy.

Package conflicts: type packageconflict { Package 1 (name, version) Package 2 (name, version) (and some magic to handle depends, conflicts, and the current mod operator) }

File conflicts: type fileconflict { String filename Location 1 (a package) Location 2 (a package OR the filesystem) }

Thoughts? I think Nagy found some code here that definitely isn't up to snuff, we just need to make sure we fix it in the right way.

That's not too far away from what it looks after Nagy's patches: struct __pmconflict_t { char package1[PKG_NAME_LEN]; char package2[PKG_NAME_LEN]; }; struct __pmfileconflict_t { char target[PKG_NAME_LEN]; char file[CONFLICT_FILE_LEN]; char ctarget[PKG_NAME_LEN]; }; But maybe it would be better to extend the package conflict structure like you said above. IMO it would mostly help for the current conflict resolving code, which is quite ugly. I think it helps less after Nagy's resolve conflict patch, which makes this code simpler / cleaner, but I would need to look more into this to be sure. About file conflict structure, it's quite similar to what you said : file is the filename target is the location 1 : a package ctarget is the location 2 : filesystem if equals to "", a package otherwise So what bothered me mostly is the actual representation of location 1 and location 2. What did you have in mind exactly?

Kevin Piche

1 Nov 1 Nov

1:25 p.m.

On Wed, 2007-10-31 at 13:42 +0100, Xavier wrote:

...

On Mon, Oct 29, 2007 at 07:23:25PM -0500, Dan McGee wrote:

...
On 10/23/07, Xavier wrote:

...
On Sun, Oct 21, 2007 at 09:33:42PM +0200, Nagy Gabor wrote:

...
Well, I overlooked something. pmconflict_t is reserved for file-conflicts only, and pmdepmissing_t is used for conflicts?! (UGLY) So here is a little patch which renames pmconflict_t to pmfileconflict_t and alpm_conflict_* to alpm_fileconflict_*.

I also found this confusing when I first saw it, so I think I like your change :)

...
pmconflicttype_t was removed from pmfileconflict_t structure, because type can be determined with (ctarget == "").

Well, I'm less sure about that, but I don't know really. What do others think?

I'm just confused overall here, and here is what I think should be done in some way or another. File conflicts and target (package) conflicts are completely different things, and should in now way be handled with the same struct.

I don't know what a ctarget is- that is a terrible name. Let's get rid of it.

I feel like we can simplify dep handling/target conflicts down to one logical group, and file conflicts down to another. The file conflicts stuff is easy; the package conflicts/depends is not so easy.

Package conflicts: type packageconflict { Package 1 (name, version) Package 2 (name, version) (and some magic to handle depends, conflicts, and the current mod operator) }

File conflicts: type fileconflict { String filename Location 1 (a package) Location 2 (a package OR the filesystem) }

Thoughts? I think Nagy found some code here that definitely isn't up to snuff, we just need to make sure we fix it in the right way.

That's not too far away from what it looks after Nagy's patches: struct __pmconflict_t { char package1[PKG_NAME_LEN]; char package2[PKG_NAME_LEN]; };

struct __pmfileconflict_t { char target[PKG_NAME_LEN]; char file[CONFLICT_FILE_LEN]; char ctarget[PKG_NAME_LEN]; };

Not a criticism since I haven't done a code review - I assume it's just legacy. I'm curious as to why package names are duped and put on multiple lists instead of pointers to package structs one per package.

...

But maybe it would be better to extend the package conflict structure like you said above. IMO it would mostly help for the current conflict resolving code, which is quite ugly. I think it helps less after Nagy's resolve conflict patch, which makes this code simpler / cleaner, but I would need to look more into this to be sure.

About file conflict structure, it's quite similar to what you said : file is the filename target is the location 1 : a package ctarget is the location 2 : filesystem if equals to "", a package otherwise

So what bothered me mostly is the actual representation of location 1 and location 2. What did you have in mind exactly?

_______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev

Aaron Griffin

4:39 p.m.

On 11/1/07, Kevin Piche wrote:

...

Not a criticism since I haven't done a code review - I assume it's just legacy. I'm curious as to why package names are duped and put on multiple lists instead of pointers to package structs one per package.

Yeah, it's legacy. And I agree with you. I think the whole complication is knowing when to free the package structure and when not to. If we keep freeing entirely up to the package cache list (excepting, of course, packages loaded from a file), then we can very easily use pointers to packages here - this will save us decent chunks of memory, especially considering the static sized strings are crap (which reminds me, I had a branch to change that... I think it went poof at some point).

Nagy Gabor

22 Oct 22 Oct

5:53 p.m.

New subject: [pacman-dev] [patch] pmconflict_t instead of pmdepmissing_t

Hi! Patch attached. Note: "replacing packages with -A and -U is not supported yet\n" "please remove '%s' first, using -Rd\n" This message in add.c doesn't cover the target<->target conflict case. Bye, ngaba

Nagy Gabor

5:59 p.m.

New subject: [pacman-dev] [patch] pmconflict_t instead of pmdepmissing_t

I forgot to mention, that this patch depends on my previous pmconflict_t->pmfileconflict_t rename patch.

Xavier

23 Oct 23 Oct

11:28 p.m.

New subject: [pacman-dev] [patch] pmconflict_t instead of pmdepmissing_t

On Mon, Oct 22, 2007 at 07:53:20PM +0200, Nagy Gabor wrote:

...

- pmdepmissing_t *miss = i->data; + pmconflict_t *conflict = i->data;

For staying more coherent with pmdepmissing / pmfileconflict names, shouldn't this be pmdepconflict ?

Nagy Gabor

24 Oct 24 Oct

8:30 a.m.

New subject: [pacman-dev] [patch] pmconflict_t instead of pmdepmissing_t

...

On Mon, Oct 22, 2007 at 07:53:20PM +0200, Nagy Gabor wrote:

...
- pmdepmissing_t *miss = i->data; + pmconflict_t *conflict = i->data;

For staying more coherent with pmdepmissing / pmfileconflict names, shouldn't this be pmdepconflict ? Well, the word "conflict" is used in alpm function-names, so I can live with pmconflict_t. Bye, ngaba

Nagy Gabor

12:09 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

Hi! Patch attached. This is a big one, you may not like it (probably doesn't worth my effort.) It (hopefully) fixes also: http://www.archlinux.org/pipermail/pacman-dev/2007-October/009666.html http://www.archlinux.org/pipermail/pacman-dev/2007-October/009655.html And I must mention this: @@ -708,8 +682,8 @@ int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, pmpkg_t *syncpkg, } if(!found) { - _alpm_log(PM_LOG_ERROR, _("cannot resolve dependencies for \"%s\" (\"%s\" is not in the package set)\n"), - miss->target, missdep->name); + _alpm_log(PM_LOG_ERROR, _("cannot resolve dependency \"%s\" of \"%s\"\n"), + missdep, miss->target);

Nagy Gabor

12:26 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

Well, I hacked a bug it to before sending: I replaced a free(foo) with FREE(foo) in the patch without testing (done with a text editor ;-); where FREE macro is undefined... You can fix this bug when you testing, sorry. Bye, ngaba

Nagy Gabor

1:13 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

The following may be surprising: I don't really like this patch. I started to work on that, and after I had done ~50% of the job, I realized that it is not so good as I expected. But when you did ~%50% of the job you won't stop and drop away the result... (see Concorde -- but sometimes this is the best decision) ;-) My main goal was to simplify the code. This goal was (imho) reached. BUT there are other things I don't like in pacman code, too: For example, functions share too few results. And my patch joined alpm_splitep + alpm_depcmp -- which is not good. Before my patch the depcmp-caller split the dependency, then it could call alpm_depcmp 5000 times on the splited dep (upgrade-requiredby fro example). After my patch alpm_splitdep is called 5000 times too, which is totally needless. But at least I found some small bugs while I worked on that; so I got some benefits too ;-) Bye, ngaba

Aaron Griffin

5:15 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

On 10/23/07, Nagy Gabor wrote:

...

The following may be surprising: I don't really like this patch. I started to work on that, and after I had done ~50% of the job, I realized that it is not so good as I expected. But when you did ~%50% of the job you won't stop and drop away the result... (see Concorde -- but sometimes this is the best decision) ;-) My main goal was to simplify the code. This goal was (imho) reached. BUT there are other things I don't like in pacman code, too: For example, functions share too few results. And my patch joined alpm_splitep + alpm_depcmp -- which is not good. Before my patch the depcmp-caller split the dependency, then it could call alpm_depcmp 5000 times on the splited dep (upgrade-requiredby fro example). After my patch alpm_splitdep is called 5000 times too, which is totally needless. But at least I found some small bugs while I worked on that; so I got some benefits too ;-)

I'm a little confused. Are you saying NOT to apply this patch?

Nagy Gabor

8:43 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

...

I'm a little confused. Are you saying NOT to apply this patch? I don't what to tell you what to do (you are free);-) I said the reasons pro and contra. I didn't do speed tests, so the "slow-down" may be not notable, I dunno. But personally, I wouldn't apply it. Bye, ngaba

Xavier

9:25 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

On Wed, Oct 24, 2007 at 03:13:33AM +0200, Nagy Gabor wrote:

...

The following may be surprising: I don't really like this patch. I started to work on that, and after I had done ~50% of the job, I realized that it is not so good as I expected. But when you did ~%50% of the job you won't stop and drop away the result... (see Concorde -- but sometimes this is the best decision) ;-) My main goal was to simplify the code. This goal was (imho) reached. BUT there are other things I don't like in pacman code, too: For example, functions share too few results. And my patch joined alpm_splitep + alpm_depcmp -- which is not good. Before my patch the depcmp-caller split the dependency, then it could call alpm_depcmp 5000 times on the splited dep (upgrade-requiredby fro example). After my patch alpm_splitdep is called 5000 times too, which is totally needless. But at least I found some small bugs while I worked on that; so I got some benefits too ;-)

I like the simplification it gives, but we indeed lose flexibility in the same time. That may not be a big deal however. But if your patch is rejected, then you should probably also provide one separate patch with only the small bug fixes.

Dan McGee

25 Oct 25 Oct

3:01 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

On 10/24/07, Xavier wrote:

...

On Wed, Oct 24, 2007 at 03:13:33AM +0200, Nagy Gabor wrote:

...
The following may be surprising: I don't really like this patch. I started to work on that, and after I had done ~50% of the job, I realized that it is not so good as I expected. But when you did ~%50% of the job you won't stop and drop away the result... (see Concorde -- but sometimes this is the best decision) ;-) My main goal was to simplify the code. This goal was (imho) reached. BUT there are other things I don't like in pacman code, too: For example, functions share too few results. And my patch joined alpm_splitep + alpm_depcmp -- which is not good. Before my patch the depcmp-caller split the dependency, then it could call alpm_depcmp 5000 times on the splited dep (upgrade-requiredby fro example). After my patch alpm_splitdep is called 5000 times too, which is totally needless. But at least I found some small bugs while I worked on that; so I got some benefits too ;-)

I like the simplification it gives, but we indeed lose flexibility in the same time. That may not be a big deal however.

But if your patch is rejected, then you should probably also provide one separate patch with only the small bug fixes.

Either Nagy or Xavier- it would be awesome if you could split this up as best as possible if it is good for inclusion. Thanks! -Dan

Nagy Gabor

11:59 a.m.

New subject: [pacman-dev] [patch] hide pmdepend_t behind alpm_depcmp

...

Either Nagy or Xavier- it would be awesome if you could split this up as best as possible if it is good for inclusion. Thanks! As both Xavier and I agreed, the concept was wrong, so I "reverted" the patch. However, the patch has a few good aspects; they will appear in this ML as new patches -- reworked to the current git (alpm_dep_get_string for example). Bye, ngaba

6036

Age (days ago)

6049

Last active (days ago)

List overview

Download

24 comments

5 participants

participants (5)

Aaron Griffin
Dan McGee
Kevin Piche
Nagy Gabor
Xavier