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

depsolving: weak-deps selection #39

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

supakeen
Copy link
Member

@supakeen supakeen commented Jul 20, 2023

We want to be able to select if weak dependencies should be installed; currently dnf-json always enables weak dependencies for the first transaction in its depsolve (this is generally the packageset for the image type); let's make this configurable on the packageset(s) (and perhaps even in blueprints).

The goal is to be able to turn off weak dependencies per image type.

@supakeen supakeen linked an issue Jul 20, 2023 that may be closed by this pull request
@supakeen
Copy link
Member Author

Initially applying this only to Fedora; I expect RHEL image builds to fail on this but the Fedora ones to come out correctly.

@supakeen supakeen force-pushed the dnf-json-weak-deps branch 2 times, most recently from 01a5c08 to 6c23a34 Compare July 20, 2023 21:18
ondrejbudai
ondrejbudai previously approved these changes Jul 21, 2023
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

This is pretty amazing, tbh! :)

internal/dnfjson/dnfjson.go Outdated Show resolved Hide resolved
@supakeen
Copy link
Member Author

supakeen commented Jul 21, 2023

I've moved setting the InstallWeakDeps property into the os pipeline. This should ensure that no images changed; we can then add a followup to set this property in the image type(s).

@achilleas-k
Copy link
Member

Good idea. It'll be nice to be able to control this from the image definitions.

We need to add InstallWeakDeps = true to all static package sets as well now right (the ones defined in pkg/distro/*/package_sets.go)?
And the dynamic build package sets (getPackageSetChain() for the build pipelines).

@supakeen
Copy link
Member Author

Good idea. It'll be nice to be able to control this from the image definitions.

We need to add InstallWeakDeps = true to all static package sets as well now right (the ones defined in pkg/distro/*/package_sets.go)? And the dynamic build package sets (getPackageSetChain() for the build pipelines).

So that was the route I initially took but it gets really murky really quickly as PackageSets are often appended to eachother and you might want PackageSet A to not have weak deps enabled and PackageSet B to have them enabled.

For that reason I made it so you don't do that and the os pipeline which puts together all the architecture/etc-dependent packages instead marks the first merged packageset as 'InstallWeakDeps'.

@achilleas-k
Copy link
Member

achilleas-k commented Jul 24, 2023

We discussed this outside the comments so I'll summarise here for the record.

Good idea. It'll be nice to be able to control this from the image definitions.
We need to add InstallWeakDeps = true to all static package sets as well now right (the ones defined in pkg/distro/*/package_sets.go)? And the dynamic build package sets (getPackageSetChain() for the build pipelines).

So that was the route I initially took but it gets really murky really quickly as PackageSets are often appended to eachother and you might want PackageSet A to not have weak deps enabled and PackageSet B to have them enabled.

In that case, these would be two package sets in the same chain ([]PackageSet), which is what we have already when we depsolve the base OS packages and then the user-selected ones.

For that reason I made it so you don't do that and the os pipeline which puts together all the architecture/etc-dependent packages instead marks the first merged packageset as 'InstallWeakDeps'.

The current behaviour makes the most sense. Say we start with a package set for a given purpose (build package set) and we know we want weak deps, so we set it to true. Anything we append to it after that can come from anywhere, but the primary purpose remains. So we should keep the logic of the Append() method the way it is now and document it in the method docstring.
Any caller that wants to make sure the package set has the right setting can always set the flag to whatever's needed after the append (or at the end of multiple appends), so I think it's okay to define one behaviour in the docstring and let callers flip it if they need to.

@supakeen supakeen marked this pull request as ready for review July 24, 2023 10:24
@supakeen
Copy link
Member Author

Wooo, it's green. Note the important bit: no images were built which means that there were no manifest changes with the code.

thozza
thozza previously approved these changes Jul 24, 2023
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

LGTM.

there is one cosmetic thing, but it can be changed in a follow up.

pkg/distro/fedora/distro.go Outdated Show resolved Hide resolved
`dnf-json` had its own logic for determining which transactions to solve
with weak dependencies enabled (only the first one). Move that logic
into the Go code to make it easier to change which package specs get
solved *with* weak deps (or without).

A `PackageSet` now contains an `InstallWeakDeps` property that is
propagated all the way down when depsolving.
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

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

Thanks!

@achilleas-k achilleas-k merged commit 2f38765 into osbuild:main Jul 24, 2023
7 checks passed
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.

The edge/iot ostree builds are pulling in weak deps
4 participants