Enable libexec artifacts #408

pmengelbert · 2024-10-21T20:38:20Z

This commit allows the user to specify artifacts to be installed under usr/libexec. I used the following documentation as a guide:

/usr/libexec

Below, you can see at a glance what the behavior of various settings will be. The following examples assume we are building a spec with top-level name: docker. The subpath field defaults to the package name.

# goes to /usr/libexec/docker/docker-compose
libexec:
  bin/docker-compose:

# goes to /usr/libexec/docker/cli-plugins/docker-compose
libexec:
  bin/docker-compose:
    subpath: docker/cli-plugins

# goes to /usr/libexec/something_else/cli-plugins/docker-compose
libexec:
  bin/docker-compose:
    subpath: something_else/cli-plugins

# goes to /usr/libexec/docker/cli-plugins/hello
libexec:
  bin/docker-compose:
    subpath: docker/cli-plugins
    name: hello

# goes to /usr/libexec/docker/hello
libexec:
  bin/docker-compose:
    name: hello

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Addresses (but does not finish) #242

Special notes for your reviewer:

frontend/rpm/template.go

This commit allows the user to specify artifacts to be installed under `usr/libexec`. I used the following documentation as a guide: https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s07.html#:~:text=Purpose,subdirectory%20under%20%2Fusr%2Flibexec%20. Below, you can see at a glance what the behavior of various settings will be. The following examples assume we are building a spec with top-level `name: docker`. The subpath field defaults to the package name. ```yaml # goes to /usr/libexec/docker/docker-compose libexec: bin/docker-compose: # goes to /usr/libexec/docker/cli-plugins/docker-compose libexec: bin/docker-compose: subpath: docker/cli-plugins # goes to /usr/libexec/something_else/cli-plugins/docker-compose libexec: bin/docker-compose: subpath: something_else/cli-plugins # goes to /usr/libexec/docker/cli-plugins/hello libexec: bin/docker-compose: subpath: docker/cli-plugins name: hello # goes to /usr/libexec/docker/hello libexec: bin/docker-compose: name: hello ``` Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

cpuguy83 · 2024-10-21T22:12:58Z

load.go

@@ -445,6 +445,13 @@ func (s *Spec) FillDefaults() {
 			s.Patches[k][i].Strip = &strip
 		}
 	}
+
+	for k, ac := range s.Artifacts.Libexec {


We're not doing this for any of the other artifact types, I think it would be good to leave this up to the underlying implementation to deal with rather than changing it in the spec.

Makes sense

Move the defaulting logic to the implementations, instead of modifying the spec at the outset. This will leave the spec unchanged. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

cpuguy83 · 2024-10-22T16:58:02Z

frontend/template.go

+
+import "github.com/Azure/dalec"
+
+func DefaultLibexecSubpath(s *dalec.Spec, k string) string {


This doesn't look right.
The way we handle this for the other types (like libs) is always <base>/<spec name>/<subpath>/<artifact name>

As an example, see how Libs are handled.

Also at this time I'd like to avoid abstracting the behavior here.

Let me double check a real system for common libexec usage, though.

Yeah, I think that's the way to go, and if someone needs something directly in /usr/libexec or somewhere else they can use a symlink.

What about when packages are renamed from upstream? For example, if we have the moby-buildx package, we want the docker-buildx binary to go into /usr/libexec/docker/cli-plugins/docker-buildx rather than /usr/libexec/moby-buildx/cli-plugins/docker-buildx. IMO we should let the user do the right thing without adding extra steps.

This doesn't look right. The way we handle this for the other types (like libs) is always <base>/<spec name>/<subpath>/<artifact name>

That seems pretty non-standard. Pretty much everything in my /usr/libexec directory is either in /usr/libexec directly, or nested under a single subdirectory bearing the main application's name. There's only a few files nested deeper than the application's name.

If someone needs something directly in /usr/libexec or somewhere else they can use a symlink.

That would violate the standard referenced in the PR description: "Applications may use a single subdirectory under /usr/libexec". If we create a symlink, we have one application (docker) with two subdirectories under /usr/libexec (/usr/libexec/docker and /usr/libexec/moby-buildx).

Maybe a separate field to overwrite the package name is worthwhile in this case.
Or just a bool like OmitAutoPackageName

Add `PackageName` to `ArtifactConfig` struct. Rearrange defaulting logic for libexec directories. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

cpuguy83 · 2024-10-22T22:07:47Z

artifacts.go

@@ -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"`


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.

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

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:

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)

libexec will be one of the times when the name is inserted

in order to avoid inserting the name you need to set the bool

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..

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.

libexec artifacts with an empty `subPath` field will be installed into `/usr/libexec` directly. Otherwise, the subpath and resolved name will be appended to `/usr/libexec/`. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

pmengelbert requested a review from a team as a code owner October 21, 2024 20:38

pmengelbert force-pushed the pmengelbert/rpm_path_macros/1 branch 2 times, most recently from d3aa46d to baa3fc3 Compare October 21, 2024 20:44

pmengelbert mentioned this pull request Oct 21, 2024

[REQ] Support Specifying Artifacts for Most Common RPM Path Macros #242

Open

12 tasks

cpuguy83 requested changes Oct 21, 2024

View reviewed changes

frontend/rpm/template.go Show resolved Hide resolved

pmengelbert force-pushed the pmengelbert/rpm_path_macros/1 branch from baa3fc3 to f758d96 Compare October 21, 2024 20:51

pmengelbert force-pushed the pmengelbert/rpm_path_macros/1 branch from f758d96 to f930ca4 Compare October 21, 2024 21:07

pmengelbert added 3 commits October 21, 2024 17:09

Add libexec capability for debian-based distros

049dd28

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add documentation for libexec artifacts

b334181

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Add tests for libexec

d832058

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

pmengelbert force-pushed the pmengelbert/rpm_path_macros/1 branch from 7afc527 to d832058 Compare October 21, 2024 22:06

cpuguy83 requested changes Oct 21, 2024

View reviewed changes

Move subpath defaulting for libexec artifacts

4ac9745

Move the defaulting logic to the implementations, instead of modifying the spec at the outset. This will leave the spec unchanged. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

cpuguy83 requested changes Oct 22, 2024

View reviewed changes

pmengelbert added 2 commits October 22, 2024 14:40

Address PR comments

dcbeca1

Add `PackageName` to `ArtifactConfig` struct. Rearrange defaulting logic for libexec directories. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Run go generate

0d4ecc2

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

cpuguy83 requested changes Oct 22, 2024

View reviewed changes

pmengelbert added 2 commits October 23, 2024 13:04

Don't insert name in path for libexec artifacts

0d1a89e

libexec artifacts with an empty `subPath` field will be installed into `/usr/libexec` directly. Otherwise, the subpath and resolved name will be appended to `/usr/libexec/`. Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

Update docs

fa22e90

Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>

cpuguy83 approved these changes Oct 23, 2024

View reviewed changes

cpuguy83 merged commit eb8c95c into Azure:main Oct 23, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable libexec artifacts #408

Enable libexec artifacts #408

pmengelbert commented Oct 21, 2024 •

edited

Loading

cpuguy83 Oct 21, 2024

pmengelbert Oct 22, 2024

pmengelbert Oct 22, 2024

cpuguy83 Oct 22, 2024

cpuguy83 Oct 22, 2024

cpuguy83 Oct 22, 2024

pmengelbert Oct 22, 2024 •

edited

Loading

pmengelbert Oct 22, 2024 •

edited

Loading

cpuguy83 Oct 22, 2024

pmengelbert Oct 22, 2024

cpuguy83 Oct 22, 2024

pmengelbert Oct 23, 2024

pmengelbert Oct 23, 2024 •

edited

Loading

cpuguy83 Oct 23, 2024

pmengelbert Oct 23, 2024


		import "github.com/Azure/dalec"

		func DefaultLibexecSubpath(s *dalec.Spec, k string) string {

Enable libexec artifacts #408

Enable libexec artifacts #408

Conversation

pmengelbert commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

pmengelbert Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert commented Oct 21, 2024 •

edited

Loading

pmengelbert Oct 22, 2024 •

edited

Loading

pmengelbert Oct 22, 2024 •

edited

Loading

pmengelbert Oct 23, 2024 •

edited

Loading