Skip to content

Conversation

@ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Jun 9, 2025

When possible (that is, when the name of the package being replaced is the same as the name of the package replacing it), omit the old package name on the 'replacing' line to save some column width.

This PR implements one of my suggestions from #785 (also mentioned at #1574 (comment)), and might be more palatable than #1574 as it doesn't remove any actual information from the output — only redundancy.

With a terminal column with of 87 (arbitrarily chosen to illustrate the differences in output with an available set of package upgrades), standard dnf5 formats like this:

$ sudo dnf --enable-repo=updates-testing upgrade java\* cockpit\*
Updating and loading repositories:
Repositories loaded.
Package                                   Arch   Version                   Reposit     
 Size
Upgrading:
 cockpit-machines                         noarch 333-1.fc42                updates 991.
6 KiB
   replacing cockpit-machines             noarch 332-1.fc42                updates 992.
3 KiB
 cockpit-podman                           noarch 107-1.fc42                updates 575.
0 KiB
   replacing cockpit-podman               noarch 106-1.fc42                updates 574.
7 KiB
 java-latest-openjdk                      x86_64 1:24.0.1.0.9-3.rolling.fc updates   1.
0 MiB
   replacing java-latest-openjdk          x86_64 1:24.0.1.0.9-1.rolling.fc updates   1.
0 MiB
 java-latest-openjdk-devel                x86_64 1:24.0.1.0.9-3.rolling.fc updates  11.
5 MiB
   replacing java-latest-openjdk-devel    x86_64 1:24.0.1.0.9-1.rolling.fc updates  11.
4 MiB
 java-latest-openjdk-headless             x86_64 1:24.0.1.0.9-3.rolling.fc updates 233.
3 MiB
   replacing java-latest-openjdk-headless x86_64 1:24.0.1.0.9-1.rolling.fc updates 238.
5 MiB
Installing:
 cockpit-files                            noarch 22-1.fc42                 updates 356.
7 KiB
   replacing cockpit-navigator            noarch 0.5.10-6.fc42             fedora    3.
1 MiB

Transaction Summary:
 Installing:         1 package
 Upgrading:          5 packages
 Replacing:          6 packages

Total size of inbound packages is 65 MiB. Need to download 65 MiB.
After this operation, 8 MiB will be freed (install 248 MiB, remove 256 MiB).
Is this ok [y/N]: n

Built from this branch, the output fails to wrap, and the fact that the upgrades are coming from updates-testing is also visible (where the -testing was entirely cut off, previously):

$ sudo ./_build_cmake/dnf5/dnf5 --disable-plugin=expired\* --enable-repo=updates-testing upgrade java\* cockpit\*
Updating and loading repositories:
Repositories loaded.
Package                        Arch   Version                   Repository         Size
Upgrading:
 cockpit-machines              noarch 333-1.fc42                updates-testi 991.6 KiB
   replacing                   noarch 332-1.fc42                updates       992.3 KiB
 cockpit-podman                noarch 107-1.fc42                updates-testi 575.0 KiB
   replacing                   noarch 106-1.fc42                updates       574.7 KiB
 java-latest-openjdk           x86_64 1:24.0.1.0.9-3.rolling.fc updates-testi   1.0 MiB
   replacing                   x86_64 1:24.0.1.0.9-1.rolling.fc updates         1.0 MiB
 java-latest-openjdk-devel     x86_64 1:24.0.1.0.9-3.rolling.fc updates-testi  11.5 MiB
   replacing                   x86_64 1:24.0.1.0.9-1.rolling.fc updates        11.4 MiB
 java-latest-openjdk-headless  x86_64 1:24.0.1.0.9-3.rolling.fc updates-testi 233.3 MiB
   replacing                   x86_64 1:24.0.1.0.9-1.rolling.fc updates       238.5 MiB
Installing:
 cockpit-files                 noarch 22-1.fc42                 updates       356.7 KiB
   replacing cockpit-navigator noarch 0.5.10-6.fc42             fedora          3.1 MiB

Transaction Summary:
 Installing:         1 package
 Upgrading:          5 packages
 Replacing:          6 packages

Total size of inbound packages is 65 MiB. Need to download 65 MiB.
After this operation, 8 MiB will be freed (install 248 MiB, remove 256 MiB).
Is this ok [y/N]: n

The output will still often wrap, especially when the terminal width is only 80 columns, but it'll wrap less and at least that's something, given that this relatively-conservative output change involves zero pain or information loss.

When possible (that is, when the name of the package being replaced is the
same as the name of the package replacing it), omit the old package name
on the 'replacing' line to save some column width.
@ferdnyc ferdnyc requested a review from a team as a code owner June 9, 2025 10:51
@ferdnyc ferdnyc requested review from evan-goode and removed request for a team June 9, 2025 10:51
std::string name;
if (pkg->get_name() == replaced->get_name()) {
// Abbreviated format for simple version upgrades
name = _("replacing");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to be nicer to translators, I could avoid a new translatable string (assuming "replacing" is a new translatable string) by making this:

Suggested change
name = _("replacing");
name = libdnf5::utils::sformat(_("replacing {}"), "");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Although, now that I think about it, that may not work for all translations?)

Copy link
Contributor

Choose a reason for hiding this comment

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

For translating it's always better to have full sentences because the translators can then reorder the words as they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, well, the full string here is "replacing"; there's nothing else. (It's a table cell, so there are no full sentences to begin with. It previously contained "replacing <package-name>".)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 9, 2025

It looks like test failures may be due to an implicit assumption in the test code that 'replacing' lines will contain a package name. Example:

  Scenario: Upgrade to RPM with same NVR but different epoch from RPM with no epoch  # dnf/epoch.feature:15
    Given I use repository "epoch"                                                   # dnf/steps/repo.py:183
    And I successfully execute dnf with args "install dummy"                         # dnf/steps/cmd.py:191
    And I use repository "epoch1"                                                    # dnf/steps/repo.py:183
    When I execute dnf with args "upgrade dummy"                                     # dnf/steps/cmd.py:109
    Then the exit code is 0                                                          # common/output.py:46
    And Transaction is following                                                     # dnf/steps/transaction.py:158
      | Action  | Package              |
      | upgrade | dummy-1:1.0-1.x86_64 |
      Traceback (most recent call last):
        File "/usr/lib/python3.13/site-packages/behave/model.py", line 1329, in run
          match.run(runner.context)
          ~~~~~~~~~^^^^^^^^^^^^^^^^
        File "/usr/lib/python3.13/site-packages/behave/matchers.py", line 98, in run
          self.func(context, *args, **kwargs)
          ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "dnf/steps/transaction.py", line 160, in then_Transaction_is_following
          check_transaction(context, 'exact_match')
          ~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^
        File "dnf/steps/transaction.py", line 155, in check_transaction
          check_dnf5_transaction(context, mode)
          ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
        File "dnf/steps/transaction.py", line 116, in check_dnf5_transaction
          dnf_transaction = parse_transaction_table_dnf5(context, lines)
        File "/opt/ci/dnf-behave-tests/dnf/steps/lib/dnf.py", line 157, in parse_transaction_table_dnf5
          rpm = RPM(nevra)
        File "/opt/ci/dnf-behave-tests/dnf/steps/lib/rpm.py", line 25, in __init__
          nevra_split = self.parse(nevra)
        File "/opt/ci/dnf-behave-tests/dnf/steps/lib/rpm.py", line 19, in parse
          raise ValueError("Cannot parse NEVRA: %s" % nevra)
      ValueError: Cannot parse NEVRA: x86_64-0:epoch.1.0-1
      
      Captured stdout:
      
      Last Command: dnf5 -y --installroot=/tmp/dnf_ci_installroot_tux1u5ax --releasever=29 --setopt=module_platform_id=platform:f29 --setopt=cachedir=/tmp/dnf_ci_installroot_tux1u5ax/var/cache/dnf upgrade dummy
      
      Last Command stdout:
      Package      Arch   Version Repository      Size
      Upgrading:
       dummy       x86_64 1:1.0-1 epoch1       0.0   B
         replacing x86_64 1.0-1   epoch        0.0   B
      
      Transaction Summary:
       Upgrading:          1 package
       Replacing:          1 package
      
      Last Command stderr:
      Updating and loading repositories:
       epoch1 test repository                 100% |   1.9 MiB/s |   2.0 KiB |  00m00s
      Repositories loaded.
      Total size of inbound packages is 6 KiB. Need to download 6 KiB.
      After this operation, 0 B extra will be used (install 0 B, remove 0 B).
      [1/1] dummy-1:1.0-1.x86_64              100% |   0.0   B/s |   6.0 KiB |  00m00s
      --------------------------------------------------------------------------------
      [1/1] Total                             100% |   0.0   B/s |   6.0 KiB |  00m00s
      Running transaction
      [1/4] Verify package files              100% |   0.0   B/s |   1.0   B |  00m00s
      [2/4] Prepare transaction               100% |   0.0   B/s |   2.0   B |  00m00s
      [3/4] Upgrading dummy-1:1.0-1.x86_64    100% |   0.0   B/s | 124.0   B |  00m00s
      [4/4] Removing dummy-0:1.0-1.x86_64     100% |  48.8 KiB/s | 100.0   B |  00m00s
      Warning: skipped OpenPGP checks for 1 package from repository: epoch1
      Complete!

That it's trying to parse "x86_64-0:epoch.1.0-1" as a NEVRA string seems to indicate that it's misreading the architecture name as the package name, and the repository name ('epoch') as the version(?), for dummy-0:1.0-1.x86_64 (the 'replaced' package).

I guess parse_transaction_table_dnf5 in the test code would have to be updated for the new format.

@ppisar
Copy link
Contributor

ppisar commented Jun 9, 2025

I personally like your output, but I'm curious what's other DNF developer's opinion.
Also this change will need updating tests in ci-dnf-stack git repository that parse an output of DNF5. They would fail like dnf/installonlypkgs.feature:16 in https://artifacts.dev.testing-farm.io/16bc5945-0cee-4f78-a8d5-6e652096a19a/work-behave-dnf58wiy95e6/plans/integration/behave-dnf5/execute/data/guest/default-0/script-00-1/output.txt.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Jun 10, 2025

I'm looking at adjusting parse_transaction_table_dnf5 in the CI code now -- it gets a little tricky because previously, every line of output stood on its own. Now with this new format, the implicit package names in many (but not all) replacing lines means the parser has to be a little bit stateful, and also more flexible.

(Primarily because the output lines are "parsed" with regular expressions, which are a terrible way to parse most things. I'm tempted to replace them with a simple Pyparsing grammar that can actually interpret the output intelligently.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants