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

build: Add minimal Makefile#16

Merged
bergwolf merged 1 commit intokata-containers:masterfrom
jodh-intel:add-makefile
Jan 26, 2018
Merged

build: Add minimal Makefile#16
bergwolf merged 1 commit intokata-containers:masterfrom
jodh-intel:add-makefile

Conversation

@jodh-intel
Copy link
Copy Markdown

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

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 james.o.hunt@intel.com

@jodh-intel
Copy link
Copy Markdown
Author

jodh-intel commented Jan 25, 2018

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
Copy Markdown

sameo commented Jan 25, 2018 •
edited by amshinde
Loading

For now:

LGTM

Approved with PullApprove

@sameo
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

@grahamwhaley grahamwhaley Jan 25, 2018

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
endif

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

@grahamwhaley grahamwhaley Jan 25, 2018

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
Copy Markdown
Author

@jodh-intel jodh-intel Jan 25, 2018

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
Copy Markdown
Author

jodh-intel commented Jan 25, 2018

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

@sameo
Copy link
Copy Markdown

sameo commented Jan 25, 2018

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

@jcvenegas
Copy link
Copy Markdown
Member

jcvenegas commented Jan 25, 2018

lgtm

egernst
egernst reviewed Jan 25, 2018
View reviewed changes
Makefile

ifeq (runv,$(KATA_RUNTIME))
RUNTIME_DIR = runv
RUNTIME_NAME = runv
Copy link
Copy Markdown
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
Copy Markdown
Member

@egernst egernst Jan 25, 2018

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
Copy Markdown
Author

@jodh-intel jodh-intel Jan 25, 2018

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
Copy Markdown
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
Copy Markdown
Member

@egernst egernst Jan 25, 2018

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
Copy Markdown

sameo commented Jan 26, 2018

@gnawux @bergwolf Please have a look.

@gnawux
Copy link
Copy Markdown
Member

gnawux commented Jan 26, 2018

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

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 <james.o.hunt@intel.com>
@bergwolf
Copy link
Copy Markdown
Member

bergwolf commented Jan 26, 2018

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

@sameo
Copy link
Copy Markdown

sameo commented Jan 26, 2018

@bergwolf #18 merged now, thanks.

@bergwolf
Copy link
Copy Markdown
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 <archana.m.shinde@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@egernst egernst egernst left review comments

@grahamwhaley grahamwhaley grahamwhaley approved these changes

Reviewers whose approvals may not affect merge requirements

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