Bug 1595917 - BaseConfig __getattr__ "cleverness" makes it unexpectedly impossible to mutate config values that appear to be lists in dnf 3+
Summary: BaseConfig __getattr__ "cleverness" makes it unexpectedly impossible to mutat...
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: dnf
Version: 29
Hardware: All
OS: Linux
unspecified
high
Target Milestone: ---
Assignee: Jaroslav Rohel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords: Triaged
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-06-27 18:47 UTC by Adam Williamson
Modified: 2018-12-05 02:33 UTC (History)
8 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2018-12-05 02:33:04 UTC


Attachments (Terms of Use)

Description Adam Williamson 2018-06-27 18:47:42 UTC
Try this:

[adamw@adam dnf (master %)]$ python3
Python 3.6.5 (default, Apr 23 2018, 22:53:50) 
[GCC 8.0.1 20180410 (Red Hat 8.0.1-0.21)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from dnf.conf import MainConf
>>> mc = MainConf()
>>> print(mc.group_package_types)
['mandatory', 'default', 'conditional']
>>> mc.group_package_types.append('foo')
>>> print(mc.group_package_types)
['mandatory', 'default', 'conditional']
>>> 

I believe the problem is the messing around with __getattr__ in BaseConfig. Specifically, this bit:

        if isinstance(value, cfg.VectorString):
            return list(value)

in this case, value *is* a cfg.VectorString. So dnf returns what is essentially a *copy* of it.

Python programmers are used to things like lists being mutable - so a programmer who sees that a config value like this is a list will naturally assume they can append to it. Indeed, we have code that does this, in the 'imgcreate' library in livecd-tools:

    repo = dnf.repo.Repo(name, parent_conf = self.conf)
    if url:
        repo.baseurl.append(_varSubstitute(url))

that worked fine with dnf 2, but doesn't work (but doesn't crash or indicate an error in any other way) with dnf 3. The call runs and returns happily, but the 'real' value of repo.baseurl does not change at all: what was changed was basically a temporary copy of its value which was immediately thrown away. This broke Rawhide composes when DNF 3 landed.

It's easy enough to work around this by doing something like:

        repo.baseurl = repo.baseurl + [_varSubstitute(url)]

but you have to know or guess what the problem is first, which is not at all obvious (it took me three hours to figure this out), and this worked before, so there may well be other code out there which does it too.

I can't, offhand, see an easy way to fix this while keeping all the overly-clever messing about that's going on. So...good luck with that. :P

Comment 1 Adam Williamson 2018-07-02 15:27:00 UTC
BTW, one possibility I thought of is just to make sure this always returns an *immutable* object. For sequences, it could return a tuple not a list. This would of course still be an 'interface change' of a kind and things that do list-specific things with the object to break, but they'd break in a much more *obvious and understandable* way. And it would be easy to document this.

Comment 2 Jan Kurik 2018-08-14 10:29:21 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 29 development cycle.
Changing version to '29'.

Comment 3 Adam Williamson 2018-09-26 17:05:27 UTC
So it seems this was addressed in DNF 3.6 by just doing what I suggested:

https://github.com/rpm-software-management/dnf/pull/1200

Doesn't seem to have been clearly documented, though.

Comment 4 Adam Williamson 2018-09-26 22:48:09 UTC
So, bcl and I have been digging into this a bit, and there's another significant way in which this is badly messed up.

jmracek posted a patch for this in lorax which looked rather odd:

https://github.com/weldr/lorax/pull/478

see, it seems to just set the tsflags values to something new and throw away any previous settings. bcl pointed out that looked wrong, and jmracek said:

"Probably we have to change a documentation also. The new implementation really appends to tsflags."

What he seems to mean by that is, well, this (this is with DNF 3.5, but 3.6 behaves the same only with tuples):

>>> import dnf.base
>>> base = dnf.base.Base()
>>> base.conf.tsflags = ['test']
>>> base.conf.tsflags
['test']
>>> base.conf.tsflags = ['test2']
>>> base.conf.tsflags
['test', 'test2']
>>> 

that is, when you think what you're doing is *setting* this tsflags value, you're really *appending* to it. That, again, is wildly against the expectations of anyone used to dealing with native Python types. This seems to be basically implemented, in current DNF, in this thing:

https://github.com/rpm-software-management/libdnf/blob/master/libdnf/conf/OptionStringListAppend.cpp

tsflags is set as one of these "OptionStringListAppend" append things, and it behaves, well, as we demonstrated above. Just about any attempt to *set* it, in fact *appends* to it. There seems to be some very specific way you can clear it, but aside from that, you're just about always appending.

This weirdness results in a mess if you try to treat these apparently-native-types like...well...native types. Obviously, because of this appending behaviour, if you try to retrieve the value, append to it, then store it - which was my initial suggestion for how apps should adapt to the 'list to tuples' change - things go weird:

>>> base = dnf.base.Base()
>>> base.conf.tsflags = ['test']
>>> _tsf = base.conf.tsflags
>>> _tsf.append("test2")
>>> base.conf.tsflags = _tsf
>>> base.conf.tsflags
['test', 'test', 'test2']

With DNF 2, for some reason, that winds up giving you ['test', 'test2', 'test', 'test2'] instead, but either way it's not at all what you'd expect.

If we try concatenation instead? Same thing:

>>> base = dnf.base.Base()
>>> base.conf.tsflags = ['test']
>>> base.conf.tsflags += ['test2']
>>> base.conf.tsflags
['test', 'test', 'test2']

So, we'd suggest this is just...fundamentally broken. I think the approach here just isn't working. You can't migrate these things out of Python into C++ types with complicated behaviours like this and still successfully pretend to Python consumers that they're Python native types. I don't see any practical way you could represent the stuff that's going on in libdnf here as some native Python type and have it actually behave as Python consumers expect. The attempt to provide backwards compatibility is a good goal, but...it's just fundamentally not working.

Unless someone smarter than me sees a way it can work, I'd suggest we just stop pretending and represent these things as some sort of custom type with an appropriate interface for the actual behaviour. If it's one of these weird Automatically Appending Vector String things, it should *look* like that to a Python consumer, not like a tuple or a list or anything else.

And then document this design, and provide a migration guide for consumers.

Comment 5 Adam Williamson 2018-09-26 23:02:33 UTC
Hmm, in fact, the 'ListAppend' weirdness predates the move to libdnf. Not sure if that makes this better or worse, but, in DNF 2.x, it was implemented right in Python, in DNF itself.

Comment 6 Adam Williamson 2018-09-26 23:55:53 UTC
Here's a thing I just figured out: appending to these values worked in DNF 2, even though it was already doing stuff with custom _set() and _get() methods and stuff, because it used properties. It used the _add_option() method which still exists but is barely used, and which ultimately (AIUI) sets the attribute with the name of the config option as a *property* with its getter and setter as the relevant option class instance's _get() and _set():

    def _add_option(self, name, optionobj):
        self._option[name] = optionobj
        # pylint: disable=W0212
        def prop_get(obj):
            return obj._option[name]._get()
        def prop_set(obj, val):
            obj._option[name]._set(val)
        setattr(type(self), name, property(prop_get, prop_set))

that sort of got lost in the rewrite to libdnf - 6a47f721a0e01098af242b8821cebdbf0a99182d - and that's why appending broke...

Comment 7 Boyd 2018-09-29 07:12:42 UTC
Not sure if the lastest update has cause this but I can no longer add 'extra' repos to my livecd.   If I remove the repo=something items in my .ks file I can run livecd-creator...  But I need those extra repos.

2018-09-28T23:31:03Z INFO Upgraded: dnf-3.5.1-1.fc29.noarch

When running livecd-creator I get the following error:

Losetup remove /dev/loop5
Traceback (most recent call last):
  File "/usr/bin/livecd-creator", line 246, in <module>
    sys.exit(main())
  File "/usr/bin/livecd-creator", line 222, in main
    creator.install()
  File "/usr/lib/python3.7/site-packages/imgcreate/creator.py", line 702, in install
    yr = dbo.addRepository(name, baseurl, mirrorlist)
  File "/usr/lib/python3.7/site-packages/imgcreate/dnfinst.py", line 185, in addRepository
    repo.baseurl = repo.baseurl + [_varSubstitute(url)]
TypeError: can only concatenate tuple (not "list") to tuple

Comment 8 Adam Williamson 2018-09-30 00:24:09 UTC
That does look related, yes, though it should only happen if you have DNF 3.6, not 3.5.1.

I've fixed it in Rawhide, not yet in F29.

https://koji.fedoraproject.org/koji/buildinfo?buildID=1147987 is the Rawhide fix.

Comment 9 Fedora Update System 2018-10-01 04:23:23 UTC
livecd-tools-25.0-11.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2018-adef81d948

Comment 10 Adam Williamson 2018-10-01 05:50:09 UTC
That update does not really fix this bug, it only adapts livecd-tools to the new dnf behaviour. Editing.

Comment 11 Fedora Update System 2018-11-22 18:56:31 UTC
libdnf-0.22.3-1.fc29 dnf-4.0.9-1.fc29 dnf-plugins-core-4.0.2-1.fc29 dnf-plugins-extras-4.0.0-1.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2018-17cbc3c616

Comment 12 Fedora Update System 2018-11-23 02:56:07 UTC
dnf-4.0.9-1.fc29, dnf-plugins-core-4.0.2-1.fc29, dnf-plugins-extras-4.0.0-1.fc29, libdnf-0.22.3-1.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-17cbc3c616

Comment 13 Fedora Update System 2018-12-05 02:33:04 UTC
dnf-4.0.9-1.fc29, dnf-plugins-core-4.0.2-1.fc29, dnf-plugins-extras-4.0.0-1.fc29, libdnf-0.22.3-1.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.