rpm-software-management / libcomps

Libcomps is alternative for yum.comps library. It's written in pure C as library and there's bindings for python2 and python3.
GNU General Public License v2.0
29 stars 37 forks source link

Expose `arch` in Python bindings #92

Closed alebastr closed 9 months ago

alebastr commented 2 years ago

See #91 for the rationale. I ended up doing that myself anyways (partially, as <group arch="..." and <groupid arch="..." are yet to be implemented).

Notes:

Fixes #91

kontura commented 2 years ago
* I don't like translating `NULL` from `comps_docpackage_arches` to `None`. But tracking what else could dereference `((PyCOMPS_Sequence*)self)->list` when it's set to `NULL` is a pain.
  The only viable alternative, IMO, is to init arches in comps_docpackage to an empty list when it's requested from the python bindings. Let me know which approach is preferred.

I would prefer the None. Looking at the new __PyCOMPS_get_arches and __PyCOMPS_set_arches - if I am not mistaken you effectively only added the one NULL check to the getter? I am not that familiar with the codebase but could the ids getter also benefit from this in case it would somehow get the NULL? Maybe you could just enhance the ids getter/setter. Perhaps we could even rename them to a more general name (get/set Sequence or List?)

* `__PyCOMPS_set_arches` lacks conversion from `list[str]`. Pretty sure I've seen the code for that in one of the source files (`__pycomps_strlist_in`?), but extracting and applying it may be a bit of a pain.

I would be fine with having that but I don't think its required at this point so its up to you.

* Was it even a good idea to reuse `libcomps.StrSeq` here? If it's the right approach, I'd prefer to also move it to a separate header+source along with get/set/conversion helpers.

It seems fine, it integrates well into the existing code. Separating it would be good.

alebastr commented 2 years ago

I would prefer the None.

Ok, I'll leave None.

Looking at the new __PyCOMPS_get_arches and __PyCOMPS_set_arches - if I am not mistaken you effectively only added the one NULL check to the getter? I am not that familiar with the codebase but could the ids getter also benefit from this in case it would somehow get the NULL? Maybe you could just enhance the ids getter/setter. Perhaps we could even rename them to a more general name (get/set Sequence or List?)

I also changed reference counting for COMPS_ObjList in the setter (Py_INCREF(value); makes the whole PyObject leak when we only need to hold a reference to the list), but that's one more thing that could (should?) be applied to the rest of the codebase.

I would be fine with having that but I don't think its required at this point so its up to you.

The write scenario looks a bit awkward without the conversion (see test changes — pkg.arches = ['x86_64'] would be more convenient), but my intended usage does not involve writes, so I'll drop the idea for now :)

* Was it even a good idea to reuse `libcomps.StrSeq` here? If it's the right approach, I'd prefer to also move it to a separate header+source along with get/set/conversion helpers.

It seems fine, it integrates well into the existing code. Separating it would be good.

Will do in the next update.

AdamWill commented 2 years ago

See https://github.com/rpm-software-management/libcomps/issues/91#issuecomment-1301148471 - I'm not sure this is actually an appropriate idea, and it's not really necessary for the thing @alebastr was trying to do.