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.

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

Spt renames #23

Merged
merged 23 commits into from
Feb 25, 2019
Merged

Spt renames #23

merged 23 commits into from
Feb 25, 2019

Conversation

ricarkol
Copy link
Contributor

  • remove solo5 as a submodule
  • point to latest rumprun, rumprun-packages, and gorump
  • change a bunch of Makefiles and Dockerfiles to use spt instead of seccomp

@lumjjb lumjjb self-assigned this Feb 21, 2019
Copy link
Member

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Some minor nits, if not, LGTM!

# rumprun can only be built with gcc-6
# XXX: Not only that, this version has to match whatever
# gcc was used to build rumprun in the host.
RUN apt-get update && \
Copy link
Member

Choose a reason for hiding this comment

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

Let's combine this with the previous two commands into 1 RUN command so it will just be 1 layer. It'll help us remove the redundant apt-get update too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

# needs to be at exactly the same place where it was built.
# (unless I'm missing something).
ARG host_rumproot
ADD rumprun-solo5 ${host_rumproot}
Copy link
Member

Choose a reason for hiding this comment

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

Dockerfile best practices: Use COPY instead of ADD. This is because ADD is overloaded.

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

ADD gorump /gorump

COPY gomaincaller.go /goapp
COPY _gorump_main.c /goapp
COPY Makefile.goapp /goapp

ENV PATH="/root/unikernels/nabla-base-build/rumprun/rumprun-solo5/bin:${PATH}"
ENV PATH=${host_rumproot}"/rumprun-solo5/bin:${PATH}"
Copy link
Member

Choose a reason for hiding this comment

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

Let's make the quotes consistent? i.e. ENV PATH="${host_rumproot}/...

source $(RUMPRUN)/obj/config-PATH.sh && cd $(GORUMP)/go/src && CGO_ENABLED=0 GOROOT_BOOTSTRAP=$(GOBOOTSTRAP) GOOS=rumprun GOARCH=amd64 ./make.bash && cd $(GOBASE)
gorump: gobootstrap rumprun FORCE
source $(RUMPRUN)/obj/config && source $(RUMPRUN)/obj/config-PATH.sh && \
cd $(GORUMP)/go/src && CGO_ENABLED=0 GOROOT_BOOTSTRAP=$(GOBOOTSTRAP) GOOS=rumprun GOARCH=amd64 ./make.bash && cd $(GOBASE)
Copy link
Member

Choose a reason for hiding this comment

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

Not really a problem for this PR, but in future for aarch64, we should make GOARCH a variable. We can open an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, didn't notice that. Was grepping for x86, will look for amd64 as well. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating an issue to discuss the proper way of having multiple archs

[[ "$output" == *"v4.3.0"* ]]
[ "$status" -eq 0 ]
}


@test "test raw node (hello)" {
nabla_run --disk="${BATS_TMPDIR}"/dummy ${TOPLEVEL}/node-base/node.nabla
nabla_run --x-exec-heap ${TOPLEVEL}/node-base/node.nabla
Copy link
Member

Choose a reason for hiding this comment

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

qn: what's the --x-exec-heap flag do? and why is it only used in the node case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes the heap executable, it's needed because of Nodejs's JIT compiler. More here: Solo5/solo5#321

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have to add it to runnc after pointing to latest there as well.

Copy link
Member

Choose a reason for hiding this comment

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

yikes... I'm curious about some details here.. Wondering if there are any other situations like this. Let's chat about this at some point!

- add a Makefile rumprun_stamp target so rumprun is not built for every base
- travis only builds node, nginx, and go
- disabled some redis and python tests, as it was taking too much time to build them
and travis times out.
- add test_curl which is used by runnc tests
@ricarkol
Copy link
Contributor Author

@lumjjb: went through your comments. Moved the multi-arch issue to #24.

Copy link
Member

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the changes.

@ricarkol ricarkol merged commit 747426c into nabla-containers:master Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants