rpm-software-management / mock

Mock is a tool for a reproducible build of RPM packages.
GNU General Public License v2.0
386 stars 235 forks source link

Cannot use config_opts[f"{config_opts.package_manager}.conf"] #1357

Open hroncok opened 7 months ago

hroncok commented 7 months ago

Short description of the problem

As a result of https://github.com/rpm-software-management/mock/pull/1332 I can no longer use:

config_opts[f"{config_opts.package_manager}.conf"] += """
# whatever
"""

In configs that include Fedora branched.

I get:

ERROR: Config error: /tmp/fedora-39-x86_64-ci.cfg: '{% if releasever|int >= 40 %}dnf5{% else %}dnf{% endif %}.conf'

Output of rpm -q mock

mock-5.5-1.fc39.noarch mock-core-configs-40.3-1.fc39.noarch python3-templated-dictionary-1.4-1.fc39.noarch

Steps to reproduce issue

Full config (in /tmp/fedora-39-x86_64-ci.cfg):

include('fedora-39-x86_64.cfg')

config_opts[f"{config_opts.package_manager}.conf"] += """
# whatever
"""

command line:

$ mock -r /tmp/fedora-39-x86_64-ci.cfg --debug-config
ERROR: Config error: /tmp/fedora-39-x86_64-ci.cfg: '{% if releasever|int >= 40 %}dnf5{% else %}dnf{% endif %}.conf'
hroncok commented 7 months ago

As a workaround, I can use config_opts["dnf.conf"] which seems to work both with dnf and dnf5.

xsuchy commented 7 months ago

1) config_opts.package_manager does not exist. config_opts is a dictionary.

2) You can use this:

include('fedora-39-x86_64.cfg')

config_opts['__jinja_expand']=True
config_opts[f"{config_opts['package_manager']}.conf"] += """                                                                                                                                 
# whatever
"""
config_opts['__jinja_expand']=False

This is because of https://github.com/xsuchy/templated-dictionary?tab=readme-ov-file#enabling-expansion which has other reason that is stored somewhere in git-log. I can dig it up if you are really curious.

This is normally not needed for values, but required for keys. I should probably document it.

hroncok commented 7 months ago

config_opts.package_manager somehow exists. I don't know how, but it does.

hroncok commented 7 months ago
>>> from templated_dictionary import TemplatedDictionary
>>> td = TemplatedDictionary()
>>> td['package_manager'] = 'dnf'
>>> td.package_manager
'dnf'
hroncok commented 7 months ago

TemplatedDictionary stores the value:key pairs in self.__dict__, making them accessible as attributes.

hroncok commented 7 months ago

As a side issue I've opened https://github.com/xsuchy/templated-dictionary/issues/4

praiskup commented 7 months ago

@xsuchy wrote:

You can use this:

include('fedora-39-x86_64.cfg')

config_opts['__jinja_expand']=True config_opts[f"{config_opts['package_manager']}.conf"] += """

Actually you can not, jinja is only supported in values, not in keys.

praiskup commented 7 months ago

Actually you can not, jinja is only supported in values, not in keys.

Wrong -> that's a f-string, not jinja. Sorry, you can use that. But ... it is unnecessary, you can simply use dnf.conf instead of abstracting the package_manager out. dnf.conf and yum.conf is completely equivalent key (these are aliased to each other).

hroncok commented 7 months ago

(This issue is not blocking me in any way. But it is a regression nevertheless. If you decide to wontfix it, I'm OK with that.)

praiskup commented 7 months ago

I'm careful to call this a regression because a) Jinja in the TemplatedDictionary keys never worked, and yet b) it is expected to use Jinja in TemplatedDictionary values. So your change in #1332 was fine from this perspective.

So such a problem happens when we use config_opts values for specifying config_opts keys. This limiting factor was always there - and nothing has changed or regressed for several Mock releases.

This admittedly was/is a serious problem, though. The copr mock-config used to provide the unfortunate output (config_opts[f"{config_opts['package_manager']}.conf"] = ...), and if cached by our user - the problem still keeps appearing somewhere in the wild.

I decided to WONTFIX this particular instance of Jinja-in-key problem before :-) because I thought there was no better way to achieve the result. But now I think there is, namely using the logic from #1347.