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

Use Verify to check required commands are present #283

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Use Verify to check required commands are present #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec15642
Add the ability to check a command for existence
eds-collabora Aug 11, 2021
67bf64b
Add a new CheckEnv stage for checking command availability
eds-collabora Aug 14, 2021
9527ef3
Add CheckEnv steps for used commands to the actions that need it
eds-collabora Aug 14, 2021
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
5 changes: 5 additions & 0 deletions action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,12 @@ func (c *DebosContext) Origin(o string) (string, bool) {
}

type Action interface {
// Verify runs first
/* FIXME verify should probably be prepare or somesuch */
Verify(context *DebosContext) error
// CheckEnv runs in the environment that Run will execute in,
// prior to Run commencing
CheckEnv(context *DebosContext) error
PreMachine(context *DebosContext, m *fakemachine.Machine, args *[]string) error
PreNoMachine(context *DebosContext) error
Run(context *DebosContext) error
Expand All @@ -75,6 +79,7 @@ type BaseAction struct {
}

func (b *BaseAction) Verify(context *DebosContext) error { return nil }
func (b *BaseAction) CheckEnv(context *DebosContext) error { return nil }
func (b *BaseAction) PreMachine(context *DebosContext,
m *fakemachine.Machine,
args *[]string) error {
Expand Down
9 changes: 9 additions & 0 deletions actions/debootstrap_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,15 @@ func (d *DebootstrapAction) Verify(context *debos.DebosContext) error {
return nil
}

func (d *DebootstrapAction) CheckEnv(context *debos.DebosContext) error {
// Check that debootstrap is available
cmd := debos.Command{}
if err := cmd.CheckExists("Debootstrap", "debootstrap", "--version"); err != nil {
return err
}
return nil
}

func (d *DebootstrapAction) PreMachine(context *debos.DebosContext, m *fakemachine.Machine, args *[]string) error {

mounts := d.listOptionFiles(context)
Expand Down
8 changes: 8 additions & 0 deletions actions/filesystem_deploy_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ func NewFilesystemDeployAction() *FilesystemDeployAction {
return fd
}

func (fd *FilesystemDeployAction) CheckEnv(context *debos.DebosContext) error {
cmd := debos.Command{}
if err := cmd.CheckExists("Cp", "cp", "--help"); err != nil {
return err
}
return nil
}

func (fd *FilesystemDeployAction) setupFSTab(context *debos.DebosContext) error {
if context.ImageFSTab.Len() == 0 {
return errors.New("Fstab not generated, missing image-partition action?")
Expand Down
34 changes: 30 additions & 4 deletions actions/image_partition_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,10 +332,7 @@ func (i ImagePartitionAction) PreMachine(context *debos.DebosContext, m *fakemac
return nil
}

func (i ImagePartitionAction) formatPartition(p *Partition, context debos.DebosContext) error {
label := fmt.Sprintf("Formatting partition %d", p.number)
path := i.getPartitionDevice(p.number, context)

func (i ImagePartitionAction) getPartitionCommand(p *Partition) []string {
cmdline := []string{}
switch p.FS {
case "vfat":
Expand Down Expand Up @@ -385,6 +382,14 @@ func (i ImagePartitionAction) formatPartition(p *Partition, context debos.DebosC
}
}
}
return cmdline
}

func (i ImagePartitionAction) formatPartition(p *Partition, context debos.DebosContext) error {
label := fmt.Sprintf("Formatting partition %d", p.number)
path := i.getPartitionDevice(p.number, context)

cmdline := i.getPartitionCommand(p)

if len(cmdline) != 0 {
cmdline = append(cmdline, path)
Expand Down Expand Up @@ -655,6 +660,27 @@ func (i ImagePartitionAction) PostMachineCleanup(context *debos.DebosContext) er
return nil
}

func (i *ImagePartitionAction) CheckEnv(context *debos.DebosContext) error {
commands := []string{
"blkid", "parted", "sfdisk", "udevadm" }

cmd := debos.Command{}
for _, c := range commands {
if err := cmd.CheckExists(strings.Title(c), c, "--help"); err != nil {
return err
}
}
for idx, _ := range i.Partitions {
p := &i.Partitions[idx]
cmdline := i.getPartitionCommand(p)
cmd := debos.Command{}
if err := cmd.CheckExists(strings.Title(cmdline[0]), cmdline[0], "-V"); err != nil {
return err
}
}
return nil
}

func (i *ImagePartitionAction) Verify(context *debos.DebosContext) error {
if len(i.GptGap) > 0 {
log.Println("WARNING: special version of parted is needed for 'gpt_gap' option")
Expand Down
8 changes: 8 additions & 0 deletions actions/ostree_deploy_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ func NewOstreeDeployAction() *OstreeDeployAction {
return ot
}

func (ot *OstreeDeployAction) CheckEnv(context *debos.DebosContext) error {
cmd := debos.Command{}
if err := cmd.CheckExists("Cp", "cp", "--help"); err != nil {
return err
}
return nil
}

func (ot *OstreeDeployAction) setupFSTab(deployment *ostree.Deployment, context *debos.DebosContext) error {
deploymentDir := fmt.Sprintf("ostree/deploy/%s/deploy/%s.%d",
deployment.Osname(), deployment.Csum(), deployment.Deployserial())
Expand Down
8 changes: 8 additions & 0 deletions actions/pack_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ func (pf *PackAction) Verify(context *debos.DebosContext) error {
pf.Compression, strings.Join(possibleTypes, ", "))
}

func (pf *PackAction) CheckEnv(context *debos.DebosContext) error {
cmd := debos.Command{}
if err := cmd.CheckExists("Tar", "tar", "--help"); err != nil {
return err
}
return nil
}

func (pf *PackAction) Run(context *debos.DebosContext) error {
usePigz := false
if pf.Compression == "gz" {
Expand Down
9 changes: 9 additions & 0 deletions actions/recipe_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ func (recipe *RecipeAction) Verify(context *debos.DebosContext) error {
return nil
}

func (recipe *RecipeAction) CheckEnv(context *debos.DebosContext) error {
for _, a := range recipe.Actions.Actions {
if err := a.CheckEnv(&recipe.context); err != nil {
return err
}
}
return nil
}

func (recipe *RecipeAction) PreMachine(context *debos.DebosContext, m *fakemachine.Machine, args *[]string) error {
// TODO: check args?

Expand Down
40 changes: 29 additions & 11 deletions cmd/debos/debos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,34 @@ func main() {
return
}

if !runInFakeMachine && !fakemachine.InMachine() {
sjoerdsimons marked this conversation as resolved.
Show resolved Hide resolved
// We will run on the host
for _, a := range r.Actions {
// Stack PostMachineCleanup methods
defer a.PostMachineCleanup(&context)

err = a.PreNoMachine(&context)
if exitcode = checkError(&context, err, a, "PreNoMachine"); exitcode != 0 {
return
}
}
}

if !runInFakeMachine {
// Either we're on the host and intend to run on it,
// or we're in the fake machine and intend to run on
// it. In the former case, PreNoMachine has just run;
// in the latter PreMachine has already run.
for _, a := range r.Actions {
err = a.CheckEnv(&context)
if exitcode = checkError(&context, err, a, "CheckEnv"); exitcode != 0 {
return
}
}
}

if runInFakeMachine {
// We're on the host and intend to run on the fakemachine
var args []string

if options.Memory == "" {
Expand Down Expand Up @@ -338,17 +365,8 @@ func main() {
return
}

if !fakemachine.InMachine() {
for _, a := range r.Actions {
// Stack PostMachineCleanup methods
defer a.PostMachineCleanup(&context)

err = a.PreNoMachine(&context)
if exitcode = checkError(&context, err, a, "PreNoMachine"); exitcode != 0 {
return
}
}
}
// We're on the host and intend to run on the host, or we're
// in the fakemachine and intend to run on the fakemachine.

// Create Rootdir
if _, err = os.Stat(context.Rootdir); os.IsNotExist(err) {
Expand Down
29 changes: 22 additions & 7 deletions commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ type Command struct {

type commandWrapper struct {
label string
writeBeforeFlush bool
Copy link
Member

Choose a reason for hiding this comment

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

can we name this in a somewhat more meaningfull way? I had to go over this a few times before i understood what this name meant ;)

buffer *bytes.Buffer
}

func newCommandWrapper(label string) *commandWrapper {
func newCommandWrapper(label string, writeBeforeFlush bool) *commandWrapper {
b := bytes.Buffer{}
return &commandWrapper{label, &b}
return &commandWrapper{label, writeBeforeFlush, &b}
}

func (w commandWrapper) out(atEOF bool) {
Expand All @@ -60,7 +61,9 @@ func (w commandWrapper) out(atEOF bool) {

func (w commandWrapper) Write(p []byte) (n int, err error) {
n, err = w.buffer.Write(p)
w.out(false)
if w.writeBeforeFlush {
w.out(false)
}
return
}

Expand Down Expand Up @@ -208,7 +211,7 @@ func (cmd *Command) restoreResolvConf(sum *[sha256.Size]byte) error {
return nil
}

func (cmd Command) Run(label string, cmdline ...string) error {
func (cmd Command) run(label string, w *commandWrapper, cmdline []string) error {
q := newQemuHelper(cmd)
q.Setup()
defer q.Cleanup()
Expand Down Expand Up @@ -241,14 +244,11 @@ func (cmd Command) Run(label string, cmdline ...string) error {
}

exe := exec.Command(options[0], options[1:]...)
w := newCommandWrapper(label)

exe.Stdin = nil
exe.Stdout = w
exe.Stderr = w

defer w.flush()

if len(cmd.extraEnv) > 0 && cmd.ChrootMethod != CHROOT_METHOD_NSPAWN {
exe.Env = append(os.Environ(), cmd.extraEnv...)
}
Expand Down Expand Up @@ -278,6 +278,21 @@ func (cmd Command) Run(label string, cmdline ...string) error {
return nil
}

func (cmd Command) Run(label string, cmdline ...string) error {
w := newCommandWrapper(label, true)
defer w.flush()
return cmd.run(label, w, cmdline)
}

func (cmd Command) CheckExists(label string, cmdline ...string) error {
w := newCommandWrapper(label, false)
Copy link
Member

Choose a reason for hiding this comment

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

is it worth being simple and just using exec.LookPath to check the executable exists somewhere rather than running the help commadn and checking the output ?

Copy link
Author

Choose a reason for hiding this comment

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

If you'd prefer, I can make that change. Actually running the command seemed safest and simplest to me; it catches things like broken installs, missing libraries, and so forth. Just the presence of an executable doesn't mean you can use it.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw i'm fine either way; I'm not sure trying to run things add much given well, so many things could potentially be broken. What if the tools are slightly different versions? what if a tool isn't bulid with the right options? etc etc. So i think going beyond, does it exist in the path has minimal extra value..

That said as things run in fakemachine that may potentially break programs, but that case seems both extremely rare and questionable if you'd detect that with just running --help.

But again either way works for me as i don't think running help on a bunch of tools will have a negative impact either ;)

if err := cmd.run(label, w, cmdline); err != nil {
w.flush()
return fmt.Errorf("Unable to find %[1]s, %[2]v", label, err)
}
return nil
}

type qemuHelper struct {
qemusrc string
qemutarget string
Expand Down