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

build: Add minimal Makefile #16

Merged
merged 1 commit into from
Jan 26, 2018
Merged

build: Add minimal Makefile #16

merged 1 commit into from
Jan 26, 2018

Conversation

jodh-intel
Copy link
Contributor

Add a basic Makefile to allow a runtime to be built:

  • Clear Containers-based Kata runtime:

    $ make KATA_RUNTIME=cc [install]

  • runv-based Kata runtime:

    $ make KATA_RUNTIME=runv [install]

Fixes #15.

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

@jodh-intel
Copy link
Contributor Author

Notes:

  • The sym link is only created for runv as the Clear Containers runtime can already be built with the name kata-runtime.

  • Clearly, the logic is very simplistic as the Makefile currently hard-codes the /usr/local/bin/kata-runtime path ( no prefix / bindir handling).

  • I'm not currently able to build the runv runtime successfully (although make is doing the right thing). It looks like a GOPATH issue maybe?

@sameo
Copy link

sameo commented Jan 25, 2018 •
edited by amshinde
Loading

For now:

LGTM

Approved with PullApprove

@sameo
Copy link

sameo commented Jan 25, 2018

For fairness we will change the cc-runtime build to generate kata-runtime-cc instead of kata-runtime and change that Makefile accordingly.

grahamwhaley
grahamwhaley approved these changes Jan 25, 2018
View reviewed changes
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.

As you noted, the CC path is hardwired and linked into how the CC runtime builds, but as a first pass to get us up and running:
lgtm

Makefile Outdated
endif

# remove goals that don't require the variable to be set
remaining=$(filter-out clean help,$(MAKECMDGOALS))
Copy link
Contributor

Choose a reason for hiding this comment

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

You scary Make guru :-)

Makefile Outdated
endif

clean:
rm -f $(KATA_RUNTIME_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could run clean in the (configured, or both?) runtime dirs as well maybe.

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.

@jodh-intel jodh-intel force-pushed the add-makefile branch 2 times, most recently from e7c0521 to 4bcefc2 Compare January 25, 2018 15:20
@jodh-intel
Copy link
Contributor Author

@sameo - the CC runtime is now built as kata-runtime-cc and sym-linked like runv.

@sameo
Copy link

sameo commented Jan 25, 2018

@gnawux @bergwolf Please let us know if that looks fine to you.

@jcvenegas
Copy link
Member

lgtm

egernst
egernst reviewed Jan 25, 2018
View reviewed changes
Makefile

ifeq (runv,$(KATA_RUNTIME))
RUNTIME_DIR = runv
RUNTIME_NAME = runv
Copy link
Member

@egernst egernst Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jodh-intel should this be kata-runtime-runv?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind -- I see that our runtime name is updated just to avoid confusion and we ultimately make the symlink regardless.

Thanks James -- looks fine to me....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, but runv's build doesn't allow the target name to be overridden unfortunately.

egernst
egernst reviewed Jan 25, 2018
View reviewed changes
Copy link
Member

@egernst egernst left a comment

Choose a reason for hiding this comment

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

LGTM

Makefile

ifeq (runv,$(KATA_RUNTIME))
RUNTIME_DIR = runv
RUNTIME_NAME = runv
Copy link
Member

Choose a reason for hiding this comment

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

Nevermind -- I see that our runtime name is updated just to avoid confusion and we ultimately make the symlink regardless.

Thanks James -- looks fine to me....

@sameo
Copy link

sameo commented Jan 26, 2018

@gnawux @bergwolf Please have a look.

@gnawux
Copy link
Member

gnawux commented Jan 26, 2018

@bergwolf is working on fix on runv side to make it could pass the makefile

@jodh-intel
build: Add minimal Makefile
6faabd2 
Add a basic `Makefile` to allow a runtime to be built:

- Clear Containers-based Kata runtime:
  ```
  $ make KATA_RUNTIME=cc [install]
  ```

- `runv`-based Kata runtime:
  ```
  $ make KATA_RUNTIME=runv [install]
  ```

Fixes kata-containers#15.

Signed-off-by: James O. D. Hunt <[email protected]>
@bergwolf
Copy link
Member

fyi, I've opened #18 to fix runv build in kata runtimes repo.

@sameo
Copy link

sameo commented Jan 26, 2018

@bergwolf #18 merged now, thanks.

@bergwolf
Copy link
Member

bergwolf commented Jan 26, 2018 •
edited by amshinde
Loading

Thanks @sameo ! That one can go in as well.

lgtm!

Approved with PullApprove

@bergwolf bergwolf merged commit 275c683 into kata-containers:master Jan 26, 2018
@jodh-intel jodh-intel mentioned this pull request Jan 30, 2018
Closed
jodh-intel pushed a commit to jodh-intel/runtimes that referenced this pull request Mar 14, 2018
@jcvenegas
Merge pull request kata-containers#16 from jcvenegas/config_go
10d031f 
clean up config.go
egernst pushed a commit to egernst/runtime that referenced this pull request Feb 9, 2021
@amshinde
vendor: Update github.com/syndtr/gocapability/capability
d387c77 
The latest runc vendoring requires this version.

Shortlog:
d983527 (HEAD -> master, origin/master, origin/HEAD) Fix capHeader.pid
type (kata-containers#16)
33e07d3 capability: Deprecate NewPid and NewFile for NewPid2 and
NewFile2 (kata-containers#14)

Signed-off-by: Archana Shinde <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@egernst egernst egernst left review comments

@grahamwhaley grahamwhaley grahamwhaley approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jodh-intel @sameo @jcvenegas @gnawux @bergwolf @grahamwhaley @egernst