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

distro-sync: Print better info message when no match #1996

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

jan-kolarik
Copy link
Member

@jan-kolarik jan-kolarik commented Sep 22, 2023

If the package specified in the argument is installed but is not available in the currently enabled repositories, distro-sync will print an error when excluding the system repository.

System repo excluding was originally added here: #1148.

The originally posted fix was to simply revert the commit mentioned above (drop filtering the packages from system repo) as it was assumed as only an "optimization" change.

With regard to the comments mentioned below, the simplest and most straightforward solution, without breaking any existing workflow, is to improve the error message. The content of the message itself is also the main problem reported by the user in the linked ticket below.

CI tests: rpm-software-management/ci-dnf-stack#1390.
Resolves: https://issues.redhat.com/browse/RHEL-7018.

@jan-kolarik
Copy link
Member Author

Note: the changes in the PR are subject to discussion, therefore marking as blocked.

@m-blaha
Copy link
Member

m-blaha commented Sep 22, 2023

Eh, we really need to provide good descriptive commit messages. Filtering out @System repo packages was introduced in PR #1148 which unfortunately only says "[goal] Exclude @System repo packages from distro_sync.", with no details. I really would like to know why the packages were excluded, but alas...

@jan-kolarik
Copy link
Member Author

"Unfortunately" the CI tests are passing, I was hoping for exploring the use case that was fixed by the original change. Anyway, I discussed the matter with @j-mracek and I will try to find less invasive solution as this issue is only about user reporting (and potentially I'll try to cover also other edges)...

@j-mracek
Copy link
Member

I found the clue.
The original patch is related to TARGETED detection
Example
Installed - nspr-0:4.35.0-9.fc38.x86_64

Available:
nspr-0:4.35.0-10.fc38.x86_64
nspr-0:4.35.0-6.fc38.x86_64

Command: dnf distrosync 'nspr < 0:4.35.0-10'

Before this patch it correctly downgrades nspr
With this PR it upgrades it.

@jan-kolarik
Copy link
Member Author

Thanks @j-mracek, I will try to cover this use case in CI tests.

@jan-kolarik
Copy link
Member Author

jan-kolarik commented Sep 22, 2023

I found the clue. The original patch is related to TARGETED detection Example Installed - nspr-0:4.35.0-9.fc38.x86_64

Available: nspr-0:4.35.0-10.fc38.x86_64 nspr-0:4.35.0-6.fc38.x86_64

Command: dnf distrosync 'nspr < 0:4.35.0-10'

Before this patch it correctly downgrades nspr With this PR it upgrades it.

So just FYI, adding the SOLVER_TARGETED flag into the libdnf's Goal::distupgrade(HySelector sltr) method makes also this use case passing (which is btw in accordance with the dnf5). But I will try to look for some other solution without changing the logic related to solver filling.

@jan-kolarik
Copy link
Member Author

jan-kolarik commented Sep 22, 2023

Another thing which feels incorrect IMO is when we pass a package which is not installed, but is available in any repository. In this case we get zero exit code with "Nothing to do. Complete!" messages, though the command should be examining the state of installed packages and I would expect the Error: No packages marked for distribution synchronization. message (which is currently used when non-existing or non-available package is passed) also there. But to make this consistent, it would also need to change the exit code to 1 which is probably also not wanted due to keeping backwards compatibility there.

Print better error message when the package specified in the argument is installed but is not available in the currently enabled repositories.

Resolves: https://issues.redhat.com/browse/RHEL-7018
@jan-kolarik jan-kolarik force-pushed the jkolarik/distrosync-disabled-repo branch from 1dffe7c to 0471799 Compare September 22, 2023 12:56
@jan-kolarik jan-kolarik changed the title distro-sync: Don't filter out the system repo packages distro-sync: Print better info message when no match Sep 22, 2023
@j-mracek
Copy link
Member

j-mracek commented Oct 4, 2023

I think that the message is much better. I think we can merge this, but do you consider the change as a final? @jan-kolarik

@j-mracek j-mracek self-assigned this Oct 4, 2023
@jan-kolarik
Copy link
Member Author

I think that the message is much better. I think we can merge this, but do you consider the change as a final? @jan-kolarik

Yes, I tried to provide the reasons in the PR description. To ensure that the logic throughout the solver's path remains unchanged and to maintain backward compatibility of the command behavior, this is the only painless solution that came to my mind.

@j-mracek j-mracek merged commit c19ce84 into master Oct 11, 2023
2 of 3 checks passed
@j-mracek j-mracek deleted the jkolarik/distrosync-disabled-repo branch October 11, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants