Skip to content

build: refactor Bazel configuration towards multiple runtime tests #160

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

Merged
merged 13 commits into from
May 7, 2021

Conversation

mathetake
Copy link
Contributor

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
@mathetake mathetake marked this pull request as ready for review April 29, 2021 12:56
@mathetake mathetake requested a review from PiotrSikora as a code owner April 29, 2021 12:56
@mathetake
Copy link
Contributor Author

will add WAVM and V8 (and WAMR) builds in following PRs.

Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Some style changes, otherwise LGTM, thanks!

Let's merge WAMR first, since I think it's easier for us to add WAMR here, than to ask WAMR folks to catch-up with our changes again.

]),
deps = [
":common_lib",
# TODO(@mathetake): Add V8 lib.
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, it's probably not worth spending time too much time on getting V8 to build, and we can wait until Bazel is officially supported in V8.

mathetake added 4 commits May 7, 2021 06:23
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
]),
deps = [
":common_lib",
# TODO(@mathetake): Add WAMR lib.
Copy link
Member

Choose a reason for hiding this comment

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

Any chance you could try to add WAMR here? Or do you prefer to leave it for a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do in a separate PR.

]),
deps = [
":common_lib",
# TODO(@mathetake): Add WAVM lib.
Copy link
Member

Choose a reason for hiding this comment

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

Any chance you could try to add WAVM here? Or do you prefer to leave it for a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do in a separate PR.

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>
Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

/wait

# See https://bytecodealliance.github.io/wasmtime/c-api/
build:linux --linkopt=-lm
build:linux --linkopt=-lpthread
build:linux --linkopt=-ldl
Copy link
Member

Choose a reason for hiding this comment

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

This .bazelrc only applies to builds in this workspace.

Shouldn't those linkopts be a part of wasmtime_lib, so that other applications that link against it could pick up them up? But even then, I think the embedding binary would need to specify them 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.

Actually, let's deal with this once it becomes a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, thanks for pointing this out. I will keep that in mind.

Copy link
Member

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Could you verify that this PR doesn't accidentally break Envoy build before merging? Thanks!

# See https://bytecodealliance.github.io/wasmtime/c-api/
build:linux --linkopt=-lm
build:linux --linkopt=-lpthread
build:linux --linkopt=-ldl
Copy link
Member

Choose a reason for hiding this comment

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

Actually, let's deal with this once it becomes a problem.

@mathetake
Copy link
Contributor Author

just verified that Envoy can be built normally against this branch. Now merging. Thanks!

@mathetake mathetake merged commit 314743b into proxy-wasm:master May 7, 2021
@mathetake mathetake deleted the bazel-refacotr branch May 7, 2021 08:15
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