Add support for $releasever_major, $releasever_minor, and limited shell expansion #800

evan-goode · 2023-08-03T22:43:06Z

Resolves #710.

Add $releasever_major and $releasever_minor: whenever the releasever var is set, automatically derive and set the releasever_major and releasever_minor vars by splitting releasever on the first dot. In the future (or now, possibly), we may also want to add releasever_patch.

Compatible with this implementation in libzypp:
https://github.com/openSUSE/libzypp/blob/d03a02d986f43266af81c73cced1b310e8615971/zypp/repo/RepoVariables.cc#L478-L495

Also adds support for ${variable:-word} and ${variable:+word} expansions of vars, similar to the functionality in POSIX shell.

${variable:-word} means if variable is unset or empty, the expansion of word is substituted. Otherwise, the value of variable is substituted.

${variable:+word} means if variable is unset or empty, nothing is substituted. Otherwise, the expansion of word is substituted.

Both support nested variable expressions; you could do ${var0:-${var1:+${var2:+$var3}}}

Zypper also supports these expansions, see here: https://doc.opensuse.org/projects/libzypp/HEAD/structzypp_1_1repo_1_1RepoVarExpand.html

include/libdnf5/conf/vars.hpp

j-mracek · 2023-08-07T05:45:59Z

libdnf5/conf/vars.cpp

+ const auto [releasever_major, releasever_minor] = split_releasever(value);
+ set("releasever_major", releasever_major, prio);
+ set("releasever_minor", releasever_minor, prio);
+ }


I am not sure whether automatic detection during the setting is the right approach. Anyway it is not wrong.

I have some concerns to priority and setting empty values.
The current code using the same priority for derived variables. Why cannot we use a lover priority? Example user define from commandline releasever_major and then releasever but it overrides releasever_major because autodetected value has the same priority.

With empty value I guess that we should not substitute $releasever_minor when releasever = f38. DNF does that in other places (when no releasever (in installroot), it does not substitute it) and I somehow feel it would be good to keep the same pattern. This behavior is different from shell, but it helps with debugging.

Hmm, playing with priorities and setting releasever_major with artificially lowered priority doesn't seem right to me. I believe the root issue here lies in the design itself - releasever_major and releasever_minor are dependent on the value of the releasever variable and should be treated as kind of "readonly" (I'd even consider disallowing independent setting, allowing changes only through releasever).

That would be possible, but it would require to implement locking mechanism for Vars, like we have for configuration and it will create an exception in the logic.

Hmm, playing with priorities and setting releasever_major with artificially lowered priority doesn't seem right to me. I believe the root issue here lies in the design itself - releasever_major and releasever_minor are dependent on the value of the releasever variable and should be treated as kind of "readonly" (I'd even consider disallowing independent setting, allowing changes only through releasever).

I agree, I think this is the better approach, I made releasever_major and releasever_minor read-only. Let me know what you think.

I think this sufficiently addresses the feedback. And this is the only feedback blocking me from merging this.

@j-mracek are we good with this now?

libdnf5/conf/vars.cpp

Conan-Kudo · 2023-08-14T21:07:05Z

@evan-goode Can you rebase this to get rid of the conflicts?

evan-goode · 2023-08-14T21:17:46Z

@evan-goode Can you rebase this to get rid of the conflicts?

Yep, rebased.

Whenever the `releasever` var is set, automatically derive and set the `releasever_major` and `releasever_minor` vars by splitting `releasever` on the first ".". Compatible with this implementation in libzypp: https://github.com/openSUSE/libzypp/blob/d03a02d986f43266af81c73cced1b310e8615971/zypp/repo/RepoVariables.cc#L478-L495 For rpm-software-management#710

include/libdnf5/conf/vars.hpp

j-mracek · 2023-08-30T07:33:55Z

libdnf5/conf/vars.cpp

 }

 void Vars::set(const std::string & name, const std::string & value, Priority prio) {
+ if (is_read_only(name)) {
+ throw RuntimeError(M_("Variable \"{}\" is read-only"), name);


Please document that it can throw an error. I have to test, but I think that it will create a problem with setvars option or other channels that can modify variables.

I've tested it and it works like expected. I am still not sure whether RuntimeError is the best exception.

I know that it is used in the rest of the module but I feel that libdnf5 exception that inherits from runtime would be more appropriate. What do you think?

I agree, I added a ReadOnlyVariableError class, and I put a /// @throw ReadOnlyVariableError in the docstring. Let me know if I should document it anywhere else.

I added a test here rpm-software-management/ci-dnf-stack#1359 to ensure read-only variables cannot be set.

Additionally I asked @jrohel about opinion for locking mechanism, because he is an author of config locking mechanism.

This is not about locking. This code implements a hard-coded list of read-only variable names. The value of these variables is not locked, they can be changed at any time, indirectly, by changing the value of another variable.

include/libdnf5/conf/vars.hpp

libdnf5/conf/vars.cpp

Adds support for ${variable:-word} and ${variable:+word} POSIX-shell-like expansions of vars. ${variable:-word} means if `variable` is unset or empty, the expansion of `word` is substituted. Otherwise, the value of `variable` is substituted. ${variable:+word} means if `variable` is unset or empty, nothing is substituted. Otherwise, the expansion of `word` is substituted. Zypper supports these expansions, see here: https://doc.opensuse.org/projects/libzypp/HEAD/structzypp_1_1repo_1_1RepoVarExpand.html For rpm-software-management#710

j-mracek · 2023-08-31T14:42:34Z

Thank you very much for the patch

Whenever the `releasever` substitution variable is set, automatically derive and set the `releasever_major` and `releasever_minor` vars by splitting `releasever` on the first ".". Companion to the DNF 5 implementation here: rpm-software-management/dnf5#800 DNF 5 issue: rpm-software-management/dnf5#710 For https://bugzilla.redhat.com/show_bug.cgi?id=1789346

Ported from the DNF 5 implementation here: rpm-software-management/dnf5#800. Adds support for ${variable:-word} and ${variable:+word} POSIX-shell-like expansions of vars. ${variable:-word} means if `variable` is unset or empty, the expansion of `word` is substituted. Otherwise, the value of `variable` is substituted. ${variable:+word} means if `variable` is unset or empty, nothing is substituted. Otherwise, the expansion of `word` is substituted. Zypper supports these expansions, see here: https://doc.opensuse.org/projects/libzypp/HEAD/structzypp_1_1repo_1_1RepoVarExpand.html For: https://bugzilla.redhat.com/show_bug.cgi?id=1789346

Requires rpm-software-management/libdnf#1622 This is the same test case used by the DNF 5 implementation: rpm-software-management/dnf5#800

Ported from the DNF 5 implementation here: rpm-software-management/dnf5#800. Adds support for ${variable:-word} and ${variable:+word} POSIX-shell-like expansions of vars. ${variable:-word} means if `variable` is unset or empty, the expansion of `word` is substituted. Otherwise, the value of `variable` is substituted. ${variable:+word} means if `variable` is unset or empty, nothing is substituted. Otherwise, the expansion of `word` is substituted. Zypper supports these expansions, see here: https://doc.opensuse.org/projects/libzypp/HEAD/structzypp_1_1repo_1_1RepoVarExpand.html For: https://bugzilla.redhat.com/show_bug.cgi?id=1789346

Requires rpm-software-management/libdnf#1622 This is the same test case used by the DNF 5 implementation: rpm-software-management/dnf5#800

Whenever the `releasever` substitution variable is set, automatically derive and set the `releasever_major` and `releasever_minor` vars by splitting `releasever` on the first ".". Companion to the DNF 5 implementation here: rpm-software-management/dnf5#800 DNF 5 issue: rpm-software-management/dnf5#710 For https://bugzilla.redhat.com/show_bug.cgi?id=1789346

evan-goode added the ready for review label Aug 3, 2023

evan-goode self-assigned this Aug 3, 2023

evan-goode mentioned this pull request Aug 3, 2023

Add test for releasever_major, releasever_minor rpm-software-management/ci-dnf-stack#1359

Merged

evan-goode removed the ready for review label Aug 3, 2023

evan-goode marked this pull request as draft August 3, 2023 23:51

j-mracek reviewed Aug 4, 2023

View reviewed changes

include/libdnf5/conf/vars.hpp Outdated Show resolved Hide resolved

j-mracek reviewed Aug 4, 2023

View reviewed changes

include/libdnf5/conf/vars.hpp Outdated Show resolved Hide resolved

j-mracek reviewed Aug 7, 2023

View reviewed changes

evan-goode force-pushed the evan-goode/releasever-major-minor branch 2 times, most recently from 9c4c3f9 to 385012e Compare August 9, 2023 02:13

evan-goode marked this pull request as ready for review August 10, 2023 18:31

evan-goode added the ready for review label Aug 10, 2023

Conan-Kudo requested changes Aug 13, 2023

View reviewed changes

libdnf5/conf/vars.cpp Outdated Show resolved Hide resolved

Conan-Kudo added changes requested and removed ready for review labels Aug 14, 2023

evan-goode force-pushed the evan-goode/releasever-major-minor branch from d0c205d to 60ae7b7 Compare August 14, 2023 21:04

evan-goode added ready for review and removed changes requested labels Aug 14, 2023

evan-goode force-pushed the evan-goode/releasever-major-minor branch from 60ae7b7 to 3975a99 Compare August 14, 2023 21:17

evan-goode requested review from Conan-Kudo and j-mracek August 21, 2023 12:49

j-mracek self-assigned this Aug 22, 2023

evan-goode force-pushed the evan-goode/releasever-major-minor branch from 3975a99 to 975b8c9 Compare August 24, 2023 19:03

Conan-Kudo approved these changes Aug 24, 2023

View reviewed changes

j-mracek reviewed Aug 30, 2023

View reviewed changes

include/libdnf5/conf/vars.hpp Outdated Show resolved Hide resolved

j-mracek reviewed Aug 30, 2023

View reviewed changes

jrohel reviewed Aug 31, 2023

View reviewed changes

include/libdnf5/conf/vars.hpp Outdated Show resolved Hide resolved

jrohel reviewed Aug 31, 2023

View reviewed changes

libdnf5/conf/vars.cpp Outdated Show resolved Hide resolved

evan-goode added 4 commits August 31, 2023 12:46

Make releasever_major, releasever_minor read-only

3ae9cb8

vars: Make split_releasever private

67c7838

vars: Throw ReadOnlyVariableError when setting read-only var

bb34919

evan-goode force-pushed the evan-goode/releasever-major-minor branch from 5e0c58b to bb34919 Compare August 31, 2023 12:53

j-mracek added this pull request to the merge queue Aug 31, 2023

Merged via the queue into rpm-software-management:main with commit 54cdeab Aug 31, 2023
6 checks passed

evan-goode mentioned this pull request Sep 18, 2023

Split $releasever to $releasever_major and $releasever_minor rpm-software-management/dnf#1989

Merged

evan-goode mentioned this pull request Sep 20, 2023

[conf] Add limited shell-style variable expansion rpm-software-management/libdnf#1622

Merged

evan-goode added a commit to evan-goode/dnf that referenced this pull request Sep 20, 2023

[conf] Add test for shell-like variable expansion

5367f13

Requires rpm-software-management/libdnf#1622 This is the same test case used by the DNF 5 implementation: rpm-software-management/dnf5#800

evan-goode mentioned this pull request Sep 20, 2023

[conf] Add test for shell-like variable expansion rpm-software-management/dnf#1992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for $releasever_major, $releasever_minor, and limited shell expansion #800

Add support for $releasever_major, $releasever_minor, and limited shell expansion #800

evan-goode commented Aug 3, 2023

j-mracek Aug 7, 2023

m-blaha Aug 7, 2023

j-mracek Aug 9, 2023

evan-goode Aug 11, 2023

Conan-Kudo Aug 24, 2023

Conan-Kudo commented Aug 14, 2023

evan-goode commented Aug 14, 2023

j-mracek Aug 30, 2023

j-mracek Aug 30, 2023

j-mracek Aug 30, 2023

evan-goode Aug 30, 2023

j-mracek Aug 31, 2023

jrohel Aug 31, 2023 •

edited

Loading

j-mracek commented Aug 31, 2023

Add support for $releasever_major, $releasever_minor, and limited shell expansion #800

Add support for $releasever_major, $releasever_minor, and limited shell expansion #800

Conversation

evan-goode commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conan-Kudo commented Aug 14, 2023

evan-goode commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrohel Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

j-mracek commented Aug 31, 2023

jrohel Aug 31, 2023 •

edited

Loading