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

DnfRepo: fix module_hotfixes keyfile priority level #885

Closed

Conversation

jlebon
Copy link
Contributor

@jlebon jlebon commented Jan 27, 2020

We were setting the runtime value of the knob instead of the repoconfig
value. This meant that a user wanting to override the config via
dnf_repo_set_module_hotfixes (which also sets the runtime value) would
have its changes clobbered.

This should fix Silverblue Rawhide rpm-ostree composes:
https://pagure.io/releng/failed-composes/issue/717
https://pagure.io/releng/failed-composes/issue/929

where we use a hack to allow rpm-ostree to install modular packages
until we fully support modules (which requires #874) and it was getting
reset because dnf_repo_download_packages wants to reset the repo
config from the keyfile. For more information, see discussions at:

#885

@cgwalters
Copy link
Collaborator

@hughsie do you remember this one?

@cgwalters
Copy link
Collaborator

cgwalters commented Jan 28, 2020

Looking at librepo history there's a whole pile of activity around then with possibly related bits, e.g.
rpm-software-management/librepo@a03aaa8 and https://github.com/rpm-software-management/librepo/commit//9f9bd95ffd36248891ffe64be1a494e38b7387b9

We were setting the runtime value of the knob instead of the repoconfig
value. This meant that a user wanting to override the config via
`dnf_repo_set_module_hotfixes` (which also sets the runtime value) would
have its changes clobbered.

This should fix Silverblue Rawhide rpm-ostree composes:
https://pagure.io/releng/failed-composes/issue/717
https://pagure.io/releng/failed-composes/issue/929

where we use a hack to allow rpm-ostree to install modular packages
until we fully support modules (which requires rpm-software-management#874) and it was getting
reset because `dnf_repo_download_packages` wants to reset the repo
config from the keyfile. For more information, see discussions at:

rpm-software-management#885
@jlebon jlebon changed the title DnfRepo: don't reload settings from keyfile before downloading DnfRepo: fix module_hotfixes keyfile priority level Jan 29, 2020
@jlebon
Copy link
Contributor Author

jlebon commented Jan 29, 2020

Hmm, OK I think I follow this a bit more now. In dnf_repo_check_internal, we set LRO_LOCAL=1, clear LRO_MIRRORLIST and set LRO_URLS to our cached rpmmd so that librepo operates directly on it to determine if it's complete & up-to-date. So naturally, we want to set it back to the original values afterwards. This explains why dnf_repo_check_internal resets the knobs to the keyfile data at the end and why cc0f5aa added LRO_LOCAL=0 there.

But anyway, I realized the root issue here was actually that the module_hotfixes knob read from the keyfile was given priority RUNTIME instead of REPOCONFIG like the others. Just fixing that fixes the issue!

(Though again, it does seem to me the call to dnf_repo_set_keyfile_data in dnf_repo_update and dnf_repo_download_packages should be dropped since we already reset them in dnf_repo_check_internal which is needed for setting up the sack and it allows callers to modify knobs before actually downloading packages. Though the justification for changing these old behaviours at this point without a use case affected by this is low.)

@jlebon
Copy link
Contributor Author

jlebon commented Jan 30, 2020

Would be good to have this in soon. Latest patch should be much simpler to review :). This will unblock Rawhide Silverblue composes, which hasn't been updated since almost a month and a half now.

@j-mracek j-mracek self-assigned this Feb 4, 2020
@j-mracek
Copy link
Member

j-mracek commented Feb 4, 2020

LGTM!

@j-mracek
Copy link
Member

j-mracek commented Feb 4, 2020

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit b4c9074 has been approved by j-mracek

@rh-atomic-bot
Copy link

⌛ Testing commit b4c9074 with merge 4a7ab08...

@jlebon
Copy link
Contributor Author

jlebon commented Feb 4, 2020

Thanks for the review!

Hmm, looks like the CI build is frozen?

2020-02-04 14:35:21,538:DEBUG: Build #1220967: running
2020-02-04 14:35:26,809:INFO: Build #1220967: succeeded

Yet all the packages have built:
https://copr.fedorainfracloud.org/coprs/rpmsoftwaremanagement/rpm-gitoverlay-1580815068.819735/packages/

Can we retry?

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: j-mracek
Pushing 4a7ab08 to master...

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.

None yet

4 participants