-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Repository class refactoring #95
base: master
Are you sure you want to change the base?
Changes from 1 commit
a01df8c
6fe6003
99f79ce
3f0f219
37f8b60
ab4ba11
25df8ed
de966e5
4895e2e
a834f94
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,9 @@ | |
#include <string_view> | ||
#include <vector> | ||
|
||
#include <glibmm/fileutils.h> | ||
#include <glibmm/keyfile.h> | ||
|
||
/** | ||
* @brief This class represents an EL .repo file as it's found in | ||
* /etc/yum.repos.d/ | ||
|
@@ -47,4 +50,60 @@ class repo { | |
static void load_repository(std::filesystem::path path); | ||
}; | ||
|
||
class repository_exception : public std::runtime_error { | ||
public: | ||
explicit repository_exception(const std::string& msg) | ||
: std::runtime_error(msg) | ||
{ | ||
} | ||
}; | ||
|
||
/** | ||
* Repository file class | ||
*/ | ||
template <class Repo> class RepoFile { | ||
dhilst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
protected: | ||
std::filesystem::path m_path; | ||
std::vector<Repo> m_repositories; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are mixing up "repository" and
This class only wraps file I/O, it must not know anything about repositories There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Like this? class File {
protected:
std::filesystem::path m_path;
public:
RepoFile(const std::filesystem::path& path)
: m_path(path)
{}
virtual void read() {}
virtual void write() {}
}; (Me including the repositories list there was a mistake... I already removed it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partially, there will be an entity named something like "File" on the namespace "cloysterhpc" that will handle basic file operations. Open, close, write, read, chmod. There's a lot of that on glib and we may use them: https://gnome.pages.gitlab.gnome.org/glibmm/classGio_1_1File.html I do believe that |
||
|
||
public: | ||
RepoFile(const std::filesystem::path& path) | ||
: m_path(path) | ||
{ | ||
} | ||
|
||
virtual void read() { } | ||
virtual void write() { } | ||
}; | ||
|
||
struct ELRepo { | ||
dhilst marked this conversation as resolved.
Show resolved
Hide resolved
|
||
std::string group; | ||
std::string name; | ||
std::optional<std::string> baseURL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
std::optional<std::string> metalink; | ||
bool enabled; | ||
bool gpgcheck; | ||
std::string gpgkey; | ||
|
||
// (P/ todos os repositórios) | ||
// std::string release; | ||
}; | ||
|
||
/** | ||
* Repository file class | ||
*/ | ||
class ELRepoFile : public RepoFile<ELRepo> { | ||
private: | ||
Glib::RefPtr<Glib::KeyFile> loadFile(); | ||
|
||
public: | ||
ELRepoFile(const std::filesystem::path& path) | ||
: RepoFile(path) | ||
{ | ||
} | ||
|
||
void read() override; | ||
void write() override; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can have an abstract There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
However the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be clear
Also, @arthurmco, add overloads to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is possible; glibmm can load keyhole data from a string. |
||
|
||
#endif // CLOYSTERHPC_REPO_H_ |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,3 +56,80 @@ void repo::load_repository(std::filesystem::path path) | |
fmt::print("KeyFile Error: {}\n", ex.what()); | ||
} | ||
} | ||
|
||
Glib::RefPtr<Glib::KeyFile> ELRepoFile::loadFile() | ||
{ | ||
try { | ||
auto file = Glib::KeyFile::create(); | ||
file->load_from_file(m_path.string()); | ||
return file; | ||
} catch (Glib::FileError& e) { | ||
throw repository_exception( | ||
std::format("Could not load repository file {} ({})", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
m_path.string(), e.what())); | ||
} | ||
} | ||
|
||
void ELRepoFile::read() | ||
{ | ||
printf("reading\n"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No... |
||
|
||
auto file = loadFile(); | ||
|
||
auto reponames = file->get_groups(); | ||
|
||
for (const auto& repogroup : reponames) { | ||
auto name = file->get_string(repogroup, "name"); | ||
|
||
if (name.empty()) { | ||
throw repository_exception(std::format( | ||
"Could not load repo name from repo '{}' at file {}", | ||
repogroup.raw(), m_path.string())); | ||
} | ||
|
||
auto metalink = (file->has_key(repogroup, "metalink")) | ||
? std::make_optional(file->get_string(repogroup, "metalink")) | ||
: std::nullopt; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optionals are really cool. Add a free function for reading an optional from keyfiles There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm an idiot. I have too much difficulty reading ternary operators. That's a technical debt in my mind. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is okay, you can read this like this inline std::optional<std::string> get_optional_string(
const KeyFile& fil,
const std::string& group,
const std::string& key)
{
if (fil->has_key(group, key)) {
return fil->get_string(group, key);
}
return std::nullopt;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The optional is got with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, but glib keyfile is not using optional here, I guess the interface is older than std::optional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes it is. But we are talking about the same issue? That our API shuould provide access with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you saying for us to extend glibmm file interface and add this as a member function? |
||
auto baseurl = (file->has_key(repogroup, "baseurl")) | ||
? std::make_optional(file->get_string(repogroup, "baseurl")) | ||
: std::nullopt; | ||
|
||
auto enabled = file->get_boolean(repogroup, "enabled"); | ||
auto gpgcheck = file->get_boolean(repogroup, "gpgcheck"); | ||
auto gpgkey = file->get_string(repogroup, "gpgkey"); | ||
|
||
ELRepo repo; | ||
repo.group = repogroup.raw(); | ||
repo.name = name.raw(); | ||
repo.metalink | ||
= metalink.transform([](const auto& v) { return v.raw(); }); | ||
repo.baseURL = baseurl.transform([](const auto& v) { return v.raw(); }); | ||
repo.enabled = enabled; | ||
repo.gpgcheck = gpgcheck; | ||
repo.gpgkey = gpgkey; | ||
m_repositories.push_back(std::move(repo)); | ||
} | ||
|
||
for (const auto& r : m_repositories) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would avoid using only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only code for toying with glibmm In real life, this would not be like this (This comment applies to every printf/fmt::format comment above) |
||
std::print("repo found '{}'\n", r.group); | ||
std::print("\tname: '{}'\n", r.name); | ||
std::print("\tbaseurl: '{}'\n", r.baseURL.value_or("null")); | ||
std::print("\tmetalink: '{}'\n", r.metalink.value_or("null")); | ||
std::print("\tenabled: {}\n", r.enabled); | ||
std::print("\tgpg: (check: {}, key: {})\n", r.gpgcheck, r.gpgkey); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this debug? If yes add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was glibmm test code I forgot to remove. |
||
} | ||
|
||
void ELRepoFile::write() | ||
{ | ||
auto file = loadFile(); | ||
|
||
for (const auto& repo : m_repositories) { | ||
file->set_string(repo.group, "name", repo.name); | ||
file->set_boolean(repo.group, "enabled", repo.enabled); | ||
file->set_boolean(repo.group, "gpgcheck", repo.gpgcheck); | ||
file->set_string(repo.group, "gpgkey", repo.gpgkey); | ||
} | ||
|
||
file->save_to_file(m_path.string()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viniciusferrao opinions on this? if we'll not be handling exceptions is there any point in creating our own exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just a WIP. But not sure. But it is a good practice to specialize the throw: https://isocpp.org/wiki/faq/exceptions#what-to-throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a WIP