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.

Sign up for GitHub

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

Enable libexec artifacts #408

Merged
merged 9 commits into from
Oct 23, 2024
Merged

Enable libexec artifacts #408

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f930ca4
Enable libexec artifacts
pmengelbert Oct 21, 2024
049dd28
Add libexec capability for debian-based distros
pmengelbert Oct 21, 2024
b334181
Add documentation for libexec artifacts
pmengelbert Oct 21, 2024
d832058
Add tests for libexec
pmengelbert Oct 21, 2024
4ac9745
Move subpath defaulting for libexec artifacts
pmengelbert Oct 22, 2024
dcbeca1
Address PR comments
pmengelbert Oct 22, 2024
0d4ecc2
Run `go generate`
pmengelbert Oct 22, 2024
0d1a89e
Don't insert name in path for libexec artifacts
pmengelbert Oct 23, 2024
fa22e90
Update docs
pmengelbert Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions artifacts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ type ArtifactConfig struct {
// Name is file or dir name to use for the artifact in the package.
// If empty, the file or dir name from the produced artifact will be used.
Name string `yaml:"name,omitempty" json:"name,omitempty"`
// PackageName is the name of the package. This can be used to replace
// `spec.Name` when destination paths are generated.
PackageName string `yaml:"packageName,omitempty" json:"packageName,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

My reasoning for suggesting a bool here is because the package name can go into SubPath and we'd just skip including the spec's package name with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next time I will clarify what you want before I implement the solution. I thought you meant that either was acceptable

Copy link
Contributor Author

@pmengelbert pmengelbert Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

tldr: I think we should just use subPath and not do anything automatically. With an empty subPath, things go directly into /usr/libexec (which is standard). Otherwise, you can set subPath to docker if you want things to go into /usr/libexec/docker or docker/cli-plugins if you want /usr/libexec/docker/cli-plugins. Detailed explanation follows.

My reasoning for suggesting a bool here is because the package name can go into SubPath and we'd just skip including the spec's package name with that.

So then in order to get the desired behavior in the example I've been using, you would need

name: moby-buildx

artifacts:
  libexec:
    bin/docker-buildx:
      subPath: docker/cli-plugins
      omitAutoPackageName: true

What I don't like about that is it kind of overloads the subPath field. When we don't need to change the package name, it would be

name: docker

artifacts:
  libexec:
    bin/docker-buildx:
      subPath: cli-plugins

Both with the bool and without, it's a leaky abstraction. The user has to know several things:

  1. the name field from the spec (which is usually at the top of the file, dozens of lines away) is inserted some times but not others (yes in /usr/lib artifacts, no in /usr/bin artifacts)
  2. libexec will be one of the times when the name is inserted
  3. in order to avoid inserting the name you need to set the bool
  4. the bool changes the meaning of the subPath field, and changes the absolute path depth.

It's too much to ask of the user IMO. Better to just have /usr/libexec be the default (since that's a pretty standard thing to do) and have the user add the application name to the subpath if they want things nested in a subdirectory..

Copy link
Member

Choose a reason for hiding this comment

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

I think the issue is its 50/50 (literally on my system it is right at the 50/50 mark).
In this case also I guess the package name may not line up exactly with what's being installed as well (beyond docker cli plugins) since we need to differentiate package names from upstreams.

Ok, it seems fine to just not use the package name in the path here (and add to the docs).
Thanks for the diligence here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

func (a *ArtifactConfig) ResolveName(path string) string {
Expand Down
11 changes: 8 additions & 3 deletions frontend/deb/debroot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
_ "embed"

"github.com/Azure/dalec"
"github.com/Azure/dalec/frontend"
"github.com/Azure/dalec/frontend/pkg/bkfs"
"github.com/moby/buildkit/client/llb"
gwclient "github.com/moby/buildkit/frontend/gateway/client"
Expand Down Expand Up @@ -457,9 +456,15 @@ func createInstallScripts(worker llb.State, spec *dalec.Spec, dir string) []llb.
sorted := dalec.SortMapKeys(spec.Artifacts.Libexec)
for _, key := range sorted {
cfg := spec.Artifacts.Libexec[key]

packageName := cfg.PackageName
if packageName == "" {
packageName = spec.Name
}

resolved := cfg.ResolveName(key)
subPath := frontend.DefaultLibexecSubpath(spec, key)
writeInstall(key, filepath.Join("/usr/libexec", subPath), resolved)
targetDir := filepath.Join(`/usr/libexec`, packageName, cfg.SubPath)
writeInstall(key, targetDir, resolved)
}
}

Expand Down
32 changes: 24 additions & 8 deletions frontend/rpm/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"text/template"

"github.com/Azure/dalec"
"github.com/Azure/dalec/frontend"
"golang.org/x/exp/maps"
"golang.org/x/exp/slices"
)
Expand Down Expand Up @@ -504,11 +503,23 @@ func (w *specWrapper) Install() fmt.Stringer {
libexecFileKeys := dalec.SortMapKeys(w.Spec.Artifacts.Libexec)
for _, k := range libexecFileKeys {
le := w.Spec.Artifacts.Libexec[k]
leCopy := dalec.ArtifactConfig{
SubPath: frontend.DefaultLibexecSubpath(w.Spec, k),
Name: le.Name,

packageName := le.PackageName
if packageName == "" {
packageName = w.Spec.Name
}

targetDir := filepath.Join(`%{buildroot}/%{_libexecdir}`, packageName, le.SubPath)
fmt.Fprintln(b, "mkdir -p", targetDir)

var targetPath string
file := le.ResolveName(k)
if !strings.Contains(file, "*") {
targetPath = filepath.Join(targetDir, file)
} else {
targetPath = targetDir + "/"
}
copyArtifact(`%{buildroot}/%{_libexecdir}`, k, &leCopy)
fmt.Fprintln(b, "cp -r", k, targetPath)
}
}

Expand Down Expand Up @@ -615,9 +626,14 @@ func (w *specWrapper) Files() fmt.Stringer {
if w.Spec.Artifacts.Libexec != nil {
dataKeys := dalec.SortMapKeys(w.Spec.Artifacts.Libexec)
for _, k := range dataKeys {
df := w.Spec.Artifacts.Libexec[k]
subPath := frontend.DefaultLibexecSubpath(w.Spec, k)
fullPath := filepath.Join(`%{_libexecdir}`, subPath, df.ResolveName(k))
le := w.Spec.Artifacts.Libexec[k]
packageName := le.PackageName
if packageName == "" {
packageName = w.Spec.Name
}

targetDir := filepath.Join(`%{_libexecdir}`, packageName, le.SubPath)
fullPath := filepath.Join(targetDir, le.ResolveName(k))
fmt.Fprintln(b, fullPath)
}
}
Expand Down
11 changes: 0 additions & 11 deletions frontend/template.go
View file
Open in desktop

This file was deleted.

7 changes: 4 additions & 3 deletions test/azlinux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,8 +1071,9 @@ Environment="KUBELET_KUBECONFIG_ARGS=--bootstrap-kubeconfig=/etc/kubernetes/boot
Name: "name_only",
},
"name_and_subpath": {
SubPath: "custom/subpath",
Name: "custom_name",
SubPath: "subpath",
Name: "custom_name",
PackageName: "custom",
},
"subpath_only": dalec.ArtifactConfig{
SubPath: "custom",
Expand All @@ -1099,7 +1100,7 @@ Environment="KUBELET_KUBECONFIG_ARGS=--bootstrap-kubeconfig=/etc/kubernetes/boot
if err := validatePathAndPermissions(ctx, ref, "/usr/libexec/custom/subpath/custom_name", 0o755); err != nil {
t.Fatal(err)
}
if err := validatePathAndPermissions(ctx, ref, "/usr/libexec/custom/subpath_only", 0o755); err != nil {
if err := validatePathAndPermissions(ctx, ref, "/usr/libexec/libexec-test/custom/subpath_only", 0o755); err != nil {
t.Fatal(err)
}
})
Expand Down
Loading