Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
59 changes: 59 additions & 0 deletions include/cloysterhpc/services/repo.h
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Expand Down Expand Up @@ -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)
{
}
};
Comment on lines +53 to +59
Copy link
Collaborator

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?

Copy link
Owner

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

Copy link
Collaborator Author

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


/**
* 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are mixing up "repository" and File class

  • Remove the Repo type parameter
  • Rename RepoFile to File
  • Remove m_repositories member

This class only wraps file I/O, it must not know anything about repositories

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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)

Copy link
Owner

@viniciusferrao viniciusferrao Jan 28, 2025

Choose a reason for hiding this comment

The 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
https://gnome.pages.gitlab.gnome.org/glibmm/group__FileUtils.html
https://gnome.pages.gitlab.gnome.org/glibmm/group__UriUtils.html

I do believe that GIO will solve almost everything.


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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base_url

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;
};
Copy link
Collaborator

@dhilst dhilst Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can have an abstract RepoFile which extends the File, adds the m_repositories generic member, and with parse unparse methods, read is super read followed by parse and write is unparse followed by super write

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::filesystem::path is convertible to std::string or const char*. We must use it. It's the right API.

However the file class that be dependent on std::filesystem::path and not the derived one.

Copy link
Collaborator

@dhilst dhilst Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear

  • parse loads data from the file into the m_repositories member
  • unparse loads the m_repositories member into the file

Also, @arthurmco, add overloads to parse and unparse which I/O into istringstream ostringstream instead of directly into the file so we can test without touching the disks. I don't know if this is possible because I don't know the interface of glibmm if it can load/unload keyfiles from strings, can it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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_
77 changes: 77 additions & 0 deletions src/services/repo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {} ({})",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmt::format.

m_path.string(), e.what()));
}
}

void ELRepoFile::read()
{
printf("reading\n");
Copy link
Owner

Choose a reason for hiding this comment

The 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;
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Owner

@viniciusferrao viniciusferrao Jan 27, 2025

Choose a reason for hiding this comment

The 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 std::nullopt should be inside a std::make_optional? I don't know.

Copy link
Collaborator

@dhilst dhilst Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is okay, you can read this like this if (file->has_key(repogroup, "metalink")) { metalink = std::make_optional(file->get_string(repogroup, "metalink")) } else { metalink = std::nullopt; }. metalink is an optional set to the metalink property if it exists in the file, or set to nullopt if not. This is pretty general, we'll want to use std::optional to represent optional values in the conf file, so I'm suggesting that we create a function for that

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;
}

Copy link
Owner

@viniciusferrao viniciusferrao Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The optional is got with the .value() accessor, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Owner

Choose a reason for hiding this comment

The 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

Yes it is. But we are talking about the same issue? That our API shuould provide access with std::optional and inside the class we encapsulate that? If that's the case yes I do agree.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid using only r as variable name. Usually we use it for well defined counters, like: i, j and k. Or for iterators, like it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this debug? If yes add #ifndef NDEBUG. Also we are using fmt::format instead and use the LOG_DEBUG directive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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());
}
Loading