[dnf5] config-manager plugin #907
Conversation
jrohel
force-pushed
the
dnf5/config_manager
branch
from
2836f4c
to
92cb568
Compare
jrohel
force-pushed
the
dnf5/config_manager
branch
2 times, most recently
from
f2bfe57
to
d594b70
Compare
jrohel
force-pushed
the
dnf5/config_manager
branch
from
d594b70
to
96cedf6
Compare
|
I put back the work in progress. I am thinking about solving a corner case. |
jrohel
force-pushed
the
dnf5/config_manager
branch
from
96cedf6
to
fd316da
Compare
|
@jrohel May I ask you to enable CI tests or to create new test cases for config-manager? |
|
|
||
| // Computes CRC32 checksum of input string. | ||
| // Slow bitwise implementation. Used to calculate the checksum of the omitted part of long URLs. | ||
| uint32_t crc32(std::string_view input) { |
There was a problem hiding this comment.
Just a question - would be possible to replace that code by something from a standard library?
There was a problem hiding this comment.
Good question. I'm wondering what library to use so I don't introduce more dependencies.
There was a problem hiding this comment.
If I understand correctly, this is used in a specific use case only for generating shorter URLs. Why not utilize a hash function (like SHA1, etc.) from the existing dependency libcrypto and extract a substring of the appropriate length from the result?
There was a problem hiding this comment.
@jan-kolarik I don't currently know why, but there is indeed a dependency on libcrypto in "dnf5.spec". There is a line: BuildRequires: pkgconfig(libcrypto).
Are we really using libcrypto in libdnf5? Yes libcurl which we depend on is compiled against libcrypto in Fedora. But libcurl also supports other crypto libraries.
There was a problem hiding this comment.
I was curious, so I've queued a testing scratch build without BuildRequires: pkgconfig(libcrypto) dependency and it's still building successfully. And when looking to the optional dependencies not included in this build, like tests and some bindings, it really seems unused in the end.
There was a problem hiding this comment.
Please don't use SHA1 for any purpose. It is not available in FIPS mode in RHEL9.
There was a problem hiding this comment.
I am not sure whether it is a good idea but what about to use SHA2 but from that long hash use only few first characters. I believe that it is available from libsolv API.
There was a problem hiding this comment.
Well, SHA-256 is also not recommended by NIST now. In general, a choice of hashing algorithm depends on what you want to use the hash for.
If it is for cryptographic purposes, then it you should go with something implemented by a certified library and be prepared to change the algorithm during a life cycle of the code (i.e. always handle hash value and hash algorithm as a pair in your data and code).
If it is for mapping data onto memory buckets, e.g. for implementing associative arrays, then choose an algorithm witch is designed for it (i.e. fair and fast). E.g. MurmurHash. MD5 and SHA-1 are needlessly slow.
If it is for error detection, then CRC can be pretty fine.
For the last to uses, it's unfortunate that they are usually implemented as a copylib library. Not as a standalone, dynamic library. It's similar to Base64.
There was a problem hiding this comment.
I believe that this part can be used like it is, it could be modify in future because the hash is not something that cannot be changed. I do not consider it as a blocking problem.
There was a problem hiding this comment.
Yesterday I learnt that zlib has crc32() and crc32_z() in its public API. From zlib.h:
uLong crc = crc32(0L, Z_NULL, 0);
while (read_buffer(buffer, length) != EOF) {
crc = crc32(crc, buffer, length);
}
if (crc != original_crc) error();
| void add_repo_from_repofile(const std::string & url, const std::filesystem::path & repo_dir); | ||
| void create_repo(const std::string & url, const std::filesystem::path & repo_dir); | ||
| void test_filepath(const std::filesystem::path & dest_path) const; | ||
| void test_if_ids_not_already_exist( |
There was a problem hiding this comment.
May I ask you for a description of the method. The name is quite self descriptive but additional information how arguments are used might be helpful.
|
I would like to open a discussion related to plural form of sub-commands: If I am not mistaken DNF5 uses singular forms like |
|
Problems related to absence of directory Creating a repository from file in emptuy installroot ends in Setvars: Errors are misleading, because the presence of directory will resolve the issue. I am suggesting to improve the situation. There are multiple options how to resolve it and the list bellow does not represent all of them.
|
|
This is not a problem, but interesting discovery - Command |
Comparing |
I would like to double-check that proposed names are the best option. After release it is difficult to modify. |
|
I am for singular form (i.e.
|
|
I am also for singular forms. I like keeping the consistency with already existing names in DNF4/DNF5 for the same or very similar behavior. I believe it will also improve the transition for users to DNF5. |
jrohel
force-pushed
the
dnf5/config_manager
branch
2 times, most recently
from
89e7f0d
to
87cbea0
Compare
It remains to solve the plural/singular form of sub-commands. |
|
Plural/singular - current state: Runnings options: Enabled running repositories:
|
|
I'm almost indifferent. I had to choose I would use a singular form. Actually if this pull request had a manual page, it would be easier to consider this question from a usability point of view. |
jrohel
force-pushed
the
dnf5/config_manager
branch
from
87cbea0
to
d6ebbd4
Compare
|
The singular form was chosen. I updated the PR. |
jrohel
force-pushed
the
dnf5/config_manager
branch
from
d6ebbd4
to
9d70e00
Compare
| // Checks if the `path` directory exists. If not, according to the `create_missing_dirs` argument, | ||
| // the directories (missing paths elements) are created or `ConfigManagerError` exception is thrown. | ||
| inline void resolve_missing_dir(const std::filesystem::path & path, bool create_missing_dirs) { | ||
| if (!std::filesystem::exists(path)) { |
There was a problem hiding this comment.
This is real nitpicking - if the path is a file, the test pass because the path exists.
$ mkdir /tmp/config-manager/etc/ -p
$ touch /tmp/config-manager/etc/dnf
$ dnf5 --installroot=/tmp/config-manager config-manager setopt gpgcheck=false
There is std::filesystem::is_directory method. But if the path is a symlink to an existing directory then it will not work according to expectation - pass.
In other cases it works perfectly.
Manage libdnf5 configuration.
Subcommands:
addrepo Add repositories from the specified configuration file
or create a new repository using user options
setopt Set configuration and repositories options
unsetopt Uset/remove configuration and repositories options
setvar Set variables
unsetvar Unset/remove variables
Note:
Main configuration:
libdnf5 reads the distribution configuration from the drop-in directory
("/usr/share/dnf5/libdnf.conf.d") and system configuration from drop-in
directory ("/etc/dnf/libdnf5.conf.d"). Last, it loads the system
configuration file (by default "/etc/dnf/dnf.conf"). The latter has
the highest priority and is modified by config-manager.
Repository configuration:
Libdnf5 loads the repositories configuration and then loads the configuration
overrides. Configuration overrides are stored in files in
the "/usr/share/dnf5/repos.override.d" and "/etc/dnf/repos.override.d"
directories. The files are sorted alphabetically. The override from the next
file overrides the previous one - the last override value wins.
The config-manager writes the repositories configuration changes to the file
"/etc/dnf/repos.override.d/99-config-manager.repo".
New option: --create-missing-dir - Allow to create missing directories Without this option, an exception is thrown if the directory is missing.
What happens when the destination repository configuration file already
exists?
By default throw an error.
--overwrite - Allow overwriting of existing repository configuration file
--add-or-replace - Allow adding or replacing a repository in the existing
configuration file
jrohel
force-pushed
the
dnf5/config_manager
branch
from
9d70e00
to
afc920d
Compare
And I created a lot of CI tests rpm-software-management/ci-dnf-stack#1402. |
| parser.add_section(repo_id); | ||
|
|
||
| // Sets the default repository name. May be overwritten with "--set=name=<name>". | ||
| parser.set_value(repo_id, "name", "created by dnf5 config-manager"); |
There was a problem hiding this comment.
What about to include repoid in the name? Proposing something like <RepoID> created by dnf5 config-manager. Keeping like it is is also OK, therefore the change is optional.
|
I would also suggest to add a warning when user want to unset a value but value is not in configuration file. Unset of option for repository should be explicitly documented in man pages, because unset only removes overrides but does not touch the original repo file. |
da5b114
CI tests rpm-software-management/ci-dnf-stack#1402
Manage libdnf5 configuration.
Subcommands:
Note:
Main configuration:
libdnf5 reads the distribution configuration from the drop-in directory
("/usr/share/dnf5/libdnf.conf.d") and system configuration from drop-in
directory ("/etc/dnf/libdnf5.conf.d"). Last, it loads the system configuration
file (by default "/etc/dnf/dnf.conf"). The latter has the highest priority
and is modified by config-manager.
Repository configuration:
Libdnf5 loads the repositories configuration and then loads the configuration
overrides. Configuration overrides are stored in files in
the "/usr/share/dnf5/repos.override.d" and "/etc/dnf/repos.override.d"
directories. The files are sorted alphabetically. The override from the next
file overrides the previous one - the last override value wins.
The config-manager writes the repositories configuration changes to the file
"/etc/dnf/repos.override.d/99-config-manager.repo".