Refactor RPM handlers and Reorganize Code for Linux targets #493
Conversation
adamperlin
force-pushed
the
adamperlin/refactor-rpm
branch
from
d834e04 to
1ca8771
Compare
…containers into consumers of the interface
adamperlin
force-pushed
the
adamperlin/refactor-rpm
branch
from
1e3f133 to
0212f81
Compare
adamperlin
force-pushed
the
adamperlin/refactor-rpm
branch
from
3a81bef to
8fd0366
Compare
frontend/linux/rpm/buildroot.go
Outdated
| @@ -0,0 +1,49 @@ | |||
| package rpm | |||
There was a problem hiding this comment.
This code hasn't changed beyond method renames
frontend/linux/rpm/distro/distro.go
Outdated
| func (cfg *Config) Handle(ctx context.Context, client gwclient.Client) (*gwclient.Result, error) { | ||
| var mux frontend.BuildMux | ||
|
|
||
| mux.Add("rpm", linux.HandlePackage(cfg), &targets.Target{ |
There was a problem hiding this comment.
We can use the root generic linux.HandlePackage for this, parameterized on the cfg
|
|
||
| type PackageInstaller func(*dnfInstallConfig, string, []string) llb.RunOption | ||
|
|
||
| type dnfInstallConfig struct { |
There was a problem hiding this comment.
We only have a dnf install config and not a tdnfInstallConfig, since in Dalec thus far every option we use is both dnf and tdnf compatible. In the future as we add more distros, we may need to rethink this portion, but if we stick to only using the options that are the same between the two, then it makes our life easier!
|
|
||
| } | ||
|
|
||
| func DnfInstall(cfg *dnfInstallConfig, releaseVer string, pkgs []string) llb.RunOption { |
There was a problem hiding this comment.
I've split out the install methods between TdnfInstall/DnfInstall maybe prematurely, in case there are differences between dnf and tdnf. In particular, I'm not sure if the separate /import-keys.sh script is needed for dnf yet, since we haven't yet added distros that use dnf.
|
@cpuguy83 I am definitely open to that! I think splitting the targets and the packaging pieces out makes a lot of sense! |
…d top-level handler helper functions) to a new targets/ dir. We now have targets/linux/deb for debian/ubuntu, targets/linux/rpm for azlinux, and targets/windows for windows. Move packaging specific code -- for example, code for building up deb and rpm spec files + utility functions for creating buildroot state to a new packaging/ dir. The only code that remains under frontend/ is specific to the frontend itself, i.e. the frontend entrypoint, target routing, helpers that are specific to all targets, helpers for solve requests to buildkit, etc.
adamperlin
force-pushed
the
adamperlin/refactor-rpm
branch
from
54ec7f2 to
cb3dd0f
Compare
…re no runtime deps for platform
|
This should be ready for another look @cpuguy83 @MorrisLaw! |
There was a problem hiding this comment.
This LGTM
I do have some nits about the package structure, but not a blocker.
For the targets package, I feel like each of the distro subpackages are uneccessary.
Instead we can have something like targets.Bookworm which would be the distro config for bookworm, as an example.
I also think the linux and windows subpackages probably aren't neccessary.
LIkewise in the packaging package.
But overall, this is a nice change.
Thank you!
|
That makes sense! I can possibly do a follow up to remove some of those subpackages! |
This PR refactors the handlers for RPM targets to be methods on a distro config, almost identical to how the DEB targets work. For example, installing build dependencies, building and testing a package, installing in a container, and the like should not vary much between rpm-based distros, so the functionality has been made into methods that encode the functionality for all rpm distros.
As part of this change, the project structure was altered to nest functionality common to RPM targets under
frontend/linux/rpm, and functionality common todebtargets has been moved tofrontend/linux/deb. ADistroConfiginterface common to both is underfrontend/linux. TheDistroConfiginterface is meant to encode what kinds of functionality any new type of distro would need to provide, such that the generic handlersHandleContainer(DistroConfig)andHandlePackage(DistroConfig)can be used fordistro/containeranddistro/<package>type targets.Special notes for your reviewer:
Many of the changed files in this PR are simply renames or changes of a single import, so the number of substantively changed files is much less than the diff shows.