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

repoquery: add --disable-modular-filtering and --location #664

Merged

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Jun 26, 2023

For: #122
For: #589
For: #366
It should also be used for: #497

Unlike the dnf4 version the package now returns a std::vector of all its remote locations.
It also takes into account pkg baseurl (if there is one) which should ensure that the returned location is always the same one which dnf (librepo) would attempt to download first.

Tests update: rpm-software-management/ci-dnf-stack#1328


std::vector<std::string> remote_locations;

auto pkg_baseurl = get_baseurl();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure why we test exclusively get_baseurl(); and then auto repo_baseurls = get_repo()->get_baseurls();.

May I ask you for explanation?

Idditionally I think that the order is not perfect one. I would recommend to add first mirrors (from metalinks, mirrorlist) and the last add baseurls.

There is a problem in librepo with order of alternatives. We are unable to handle together baseurls and mirrorlists/metalinks. When baseurl is set it use only baseurl. I think baseurl should be used as a last option not the first one to split downloads to multiple providers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are actually two baseurls:

  • repo baseurl, configured in repo file. This is the base url for the whole repo (alternative repo source). You are correct about librepo I will switch the order of the loops.
  • second is package baseurl, this can theoretically be different for each package. It looks like:
<package type="rpm">
  <name>CQRlib</name>
  <arch>src</arch>
  <version epoch="0" ver="1.1.1" rel="4.fc29"/>
  <checksum type="sha256" pkgid="YES">36eb2f6e63d3df64e61b14e570d35997a0f9357414ac1f6a637c48b971f968e5</checksum>
  <summary>ANSI C API for quaternion arithmetic and rotation</summary>
  <description>CQRlib is an ANSI C implementation of a utility library for quaternion
arithmetic and quaternion rotation math.</description>
  <url>http://cqrlib.sourceforge.net/</url>
  <size package="7780" installed="1628" archive="1888"/>
  <location xml:base="http://localhost:43223" href="src/CQRlib-1.1.1-4.fc29.src.rpm"/>    <-----------
  <format>
    <rpm:license>LGPLv2+</rpm:license>
    <rpm:vendor>Fedora Project</rpm:vendor>
    <rpm:group>System Environment/Libraries</rpm:group>
    <rpm:buildhost>77a43894e301</rpm:buildhost>
    <rpm:sourcerpm></rpm:sourcerpm>
    <rpm:header-range start="4504" end="6850"/>
    <rpm:provides>
      <rpm:entry name="CQRlib" flags="EQ" epoch="0" ver="1.1.1" rel="4.fc29"/>
    </rpm:provides>
  </format>
</package>

I was thinking that if a repo says the package is elsewhere it probably doesn't make sense to try to find it in the repo location. Therefore there is a separate check for package baseurl and if present it returns only the one location.

@kontura
Copy link
Contributor Author

kontura commented Aug 22, 2023

Rebased to resolve a conflict.

@@ -327,6 +327,10 @@ class Repo {
// @replaces libdnf:repo/Repo.hpp:method:Repo.getMirrors()
std::vector<std::string> get_mirrors() const;

/// Returns baseurls of repository
/// @return A vector of baseurls
std::vector<std::string> get_baseurls() const { return config.get_baseurl_option().get_value(); };
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curios whether we need that. If I am not mistaken we already has several option like that, but do we want to make the same pattern for all configuration?

On the other hand it is not wrong and it might be useful if the option is called often.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not needed, I have dropped it.
We can always add it later if we want.

@j-mracek j-mracek self-assigned this Aug 22, 2023
@j-mracek
Copy link
Contributor

LGTM

@j-mracek j-mracek added this pull request to the merge queue Aug 22, 2023
Merged via the queue into rpm-software-management:main with commit 9f16426 Aug 22, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants