Skip to content

Remove make vmcheck wrapper#1958

Closed
cgwalters wants to merge 1 commit intocoreos:masterfrom
cgwalters:remove-make-vmcheck
Closed

Remove make vmcheck wrapper#1958
cgwalters wants to merge 1 commit intocoreos:masterfrom
cgwalters:remove-make-vmcheck

Conversation

@cgwalters
Copy link
Member

Using make as an entrypoint works best when the extended target
needs to interact somehow with building the software.
We need make check to be a target becuase it actually does
build code.

But for vmcheck that's not true. And in particular using
make targets is awkward when one wants
to configure/shortcut what the target is doing, as we have with
the environment variables SKIP_VMOVERLAY etc.

I think it's just cleaner and more obvious to have a script to
execute - and this script can have command line arguments.

All this does is remove the make vmcheck wrapper, but I
want to rewrite the entrypoint script to have e.g.
./tests/vmcheck -O where -O means "skip overlay".

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters
Copy link
Member Author

(There's a lot of other cruft around documentation and the move to cosa that I didn't try to address here)

@jlebon
Copy link
Member

jlebon commented Dec 16, 2019

Hmm, let's back up a bit here.

Here's how I see the build system integrating with cosa:

  • make -> build binaries
  • make vmoverlay -> take built binaries, call out to cosa, and overlay them to create a new image
  • make vmcheck -> use image with overlaid binaries (or just directly if SKIP_VMOVERLAY=1 is set)

So in that picture, make vmoverlay does interact with building the software. And make vmcheck takes the output from make vmoverlay to run the testsuite.

Another way to say this is, I agree we want to make vmcheck.sh a good entrypoint, but let's have the build system simply be a user of that script? Using switches instead of environment variables WFM.

More concretely, make vmcheck can simply be e.g. vmcheck.sh --image tests/vmcheck/image.qcow2.

@cgwalters
Copy link
Member Author

So in that picture, make vmoverlay does interact with building the software.

Yes...but then it goes off and does a whole lot of other stuff unrelated to that.

Bigger picture I think the way I'd like to see things go is more that cosa drives things rather than each project scripting cosa.

For example, this would enforce more uniformity among projects and also make it more straightforward to build/test multiple projects simultaneously.

It'd also paves the way for having rpm-ostree's test suite tests run indepenenently of building it.

@cgwalters
Copy link
Member Author

A good hint of how hacky things here are is that running make vmcheck only indirectly goes and does a build because of the make install invocation down in a few layers of shell script - the dependency here isn't visible to make, and it actually can't be because it's conditional on whether we want to do the build versus test something that exists.

@cgwalters
Copy link
Member Author

IOW, we're not actually really using make for this at all - the wrappers here aren't helpful versus just directly documenting running the script today anyways.

Using `make` as an entrypoint works best when the extended target
needs to interact somehow with building the software.
We need `make check` to be a target becuase it actually does
build code.

But for `vmcheck` that's not true.  And in particular using
`make` targets is awkward when one wants
to configure/shortcut what the target is doing, as we have with
the environment variables `SKIP_VMOVERLAY` etc.

I think it's just cleaner and more obvious to have a script to
execute - and this script can have command line arguments.

All this does is remove the `make vmcheck` wrapper, but I
want to rewrite the entrypoint script to have e.g.
`./tests/vmcheck -O` where `-O` means "skip overlay".
@jlebon
Copy link
Member

jlebon commented Dec 16, 2019

It'd also paves the way for having rpm-ostree's test suite tests run indepenenently of building it.

That's already the case today. (Part of #1949 was doing this decoupling so that CI didn't have to build the tree first).

A good hint of how hacky things here are is that running make vmcheck only indirectly goes and does a build because of the make install invocation down in a few layers of shell script - the dependency here isn't visible to make, and it actually can't be because it's conditional on whether we want to do the build versus test something that exists.

Again, the idea with make vmoverlay/vmcheck is to enable iterating locally via cosa dev-overlay, which needs more work today. In which case, having vmcheck depend on vmoverlay and having that do make install is very much part of the idea.

If you're using SKIP_VMOVERLAY, there's indeed no point in using it vs calling the script directly (notice for example CI doesn't do this, because it has a freshly built FCOS with the built binaries). I'd have no issues dropping support for SKIP_VMOVERLAY.

Bigger picture I think the way I'd like to see things go is more that cosa drives things rather than each project scripting cosa.

For example, this would enforce more uniformity among projects and also make it more straightforward to build/test multiple projects simultaneously.

I see where you're coming from, though I think there's also a lot of value in not having to context switch between working directories and tools when hacking. If I'm trying to get a test working, I want to be able to just hack on the rpm-ostree source code/tests, then immediately make && make vmcheck TESTS=....

Anyway, I'm not opposed to trying this out. I think time will tell where the pain points are as cosa and workflows mature. In that case though, there's really no point in having overlay.sh at all, right? Let's just nuke it entirely and for now have vmcheck.sh just assume there's a tests/vmcheck/image.qcow2 present (and in a follow-up we can have it take options).

@cgwalters
Copy link
Member Author

Closing in favor of momentum in #2072

@cgwalters cgwalters closed this Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants