Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

build: Allow for kata project build #933

Merged

Conversation

jodh-intel
Copy link
Contributor

@jodh-intel jodh-intel commented Jan 17, 2018

Rework the build system to allow the runtime to be built for either
of the following:

  • Clear Containers (cc-runtime binary and configuration)
  • Kata Containers (kata-runtime binary and configuration)

The existing build rules are retained but there are now two new rules:

$ make build-kata-system
$ sudo make install-kata-system

Two new variables have also been added:

  • KATA_SYSTEM_BUILD: set to any value to build/install for Kata.
  • SYSTEM_BUILD_TYPE: build for either Clear Containers (set to cc) or
    Kata (set to kata).

See make help for further details.

Note that configuration files are installed to a project-specific
directory, allowing for systems to be installed for both Kata and Clear
Containers.

Finally, the bash completion and collection scripts have been updated
to work for either project.

Fixes #919, #921.

Signed-off-by: James O. D. Hunt [email protected]

@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@jodh-intel jodh-intel force-pushed the build-using-project-name branch from f63d5d2 to 71ff845 Compare January 17, 2018 10:35
@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@jodh-intel jodh-intel force-pushed the build-using-project-name branch from 71ff845 to 8be1ed7 Compare January 17, 2018 11:08
@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@devimc
Copy link

devimc commented Jan 17, 2018

good patch!
lgtm

Approved with PullApprove Approved with PullApprove

@jodh-intel jodh-intel force-pushed the build-using-project-name branch from 8be1ed7 to f5c282c Compare January 17, 2018 16:01
@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@jodh-intel
Copy link
Contributor Author

Could I get another review on this PR please?

@jodh-intel jodh-intel force-pushed the build-using-project-name branch from f5c282c to 6cc2899 Compare January 18, 2018 11:20
@jodh-intel
Copy link
Contributor Author

ping @sboeuf, @sameo.

@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@jodh-intel jodh-intel force-pushed the build-using-project-name branch from 6cc2899 to 4898b84 Compare January 18, 2018 12:03
@clearcontainersbot
Copy link

kubernetes qa-passed 👍

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

That's a lot of changes :-) I've not run-tested, but from a perusal:
lgtm

I won't be surprised if we need some tweaking after merge though.

Makefile Outdated

ifneq ($(SYSTEM_BUILD_TYPE),)
ifeq ($(SYSTEM_BUILD_TYPE),$(CC_TYPE))
system_build_type = $(SYSTEM_BUILD_TYPE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Although equivalent, I think it looks less confusing to do:

system_build_type = $(CC_TYPE)

here, and the equiv for KATA below.

I guess all we are really doing here is checking that the SYSTEM_BUILD_TYPE is actually a type we know about. In which case, would an error case catchall of something like ifeq ($(SYSTEM_BUILD_TYPE),) after these two cases make sense? (that is, nobody set the type?) But, maybe that does not mesh with the other methods of choosing which sort of build (I sort of wish we only had one method....)

@@ -21,31 +21,114 @@ for file in /etc/os-release /usr/lib/os-release; do \
fi \
done)

# A "CC system build" is either triggered by specifying
# a special target or via an environment variable.
#------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Update copyright year in this file? - and check others?

Rework the build system to allow the runtime to be built for either
of the following:

- Clear Containers (`cc-runtime` binary and configuration)
- Kata Containers (`kata-runtime` binary and configuration)

The existing build rules are retained but there are now two new rules:

```
$ make build-kata-system
$ sudo make install-kata-system
```

Two new variables have also been added:

- `KATA_SYSTEM_BUILD`: set to any value to build/install for Kata.
- `SYSTEM_BUILD_TYPE`: build for either Clear Containers (set to `cc`) or
  Kata (set to `kata`).

See `make help` for further details.

Note that configuration files are installed to a project-specific
directory, allowing for systems to be installed for both Kata and Clear
Containers.

Finally, the bash completion and collection scripts have been updated
to work for either project.

Fixes clearcontainers#919, clearcontainers#921.

Signed-off-by: James O. D. Hunt <[email protected]>
@jodh-intel jodh-intel force-pushed the build-using-project-name branch from 4898b84 to 7a9a492 Compare January 18, 2018 14:42
@jodh-intel
Copy link
Contributor Author

Hi @grahamwhaley - branch updated.

@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@jodh-intel jodh-intel requested a review from sboeuf January 18, 2018 16:44
@sameo
Copy link

sameo commented Jan 18, 2018

LGTM

Approved with PullApprove Approved with PullApprove

@jodh-intel jodh-intel merged commit 804cc6c into clearcontainers:master Jan 18, 2018
jcvenegas pushed a commit to jcvenegas/cc-runtime that referenced this pull request Jan 18, 2018
…oject-name

build: Allow for kata project build
@sboeuf
Copy link
Contributor

sboeuf commented Jan 19, 2018

@jodh-intel FWIW, I have been testing Kata Containers with this PR (now in master), and I confirm it works very well as expected :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants