Skip to content

bazel: Add support for bzlmod#96

Merged
adisuissa merged 2 commits intocncf:mainfrom
keith:ks/bazel-add-support-for-bzlmod
Jul 23, 2024
Merged

bazel: Add support for bzlmod#96
adisuissa merged 2 commits intocncf:mainfrom
keith:ks/bazel-add-support-for-bzlmod

Conversation

@keith
Copy link
Copy Markdown
Contributor

@keith keith commented May 24, 2024

Based on #95

@keith keith force-pushed the ks/bazel-add-support-for-bzlmod branch from 78ded72 to adefe92 Compare May 24, 2024 00:05
@keith
Copy link
Copy Markdown
Contributor Author

keith commented May 24, 2024

cc @mmorel-35

@mmorel-35
Copy link
Copy Markdown
Contributor

mmorel-35 commented May 24, 2024

Don’t forget to regenerate go files.
Envoy still uses 6.5.0 bazel version. This requires 7.X, it might a blocker ?

@phlax , @adisuissa , WDYT ?

@phlax
Copy link
Copy Markdown
Member

phlax commented May 24, 2024

Envoy still uses 6.5.0 bazel version. This requires 7.X, it might a blocker ?

i may be wrong - but when i tried to bump envoy -> 7.x it seemed as though it also required a newer llvm version

not sure if updating would block envoy tho

@keith
Copy link
Copy Markdown
Contributor Author

keith commented May 24, 2024

Oh yea my intent wasn't to force an envoy bump here, this should continue to be backwards compatible and envoy can include it like normal. the issue that there's a transitive dep on rules_jvm_external which requires bazel 7.x

deps = depset([_go_proto_mapping(dep) for dep in deps] + [
"@com_envoyproxy_protoc_gen_validate//validate:go_default_library",
"@org_golang_google_genproto_googleapis_api//annotations:annotations",
"@org_golang_google_genproto_googleapis_rpc//status:status",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

these deps appeared unused, is building them in this repo enough to prove that we don't need them? or could something else be relying on these?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You shall try it integrated in envoy .
See https://github.com/envoyproxy/envoy/pull/33084/files for example

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

keith added 2 commits May 24, 2024 10:26
Based on cncf#95

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith keith force-pushed the ks/bazel-add-support-for-bzlmod branch from 93236f9 to 83071f3 Compare May 24, 2024 17:27
@@ -1 +1,2 @@
bazel-*
MODULE.bazel.lock
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@keith raising this here as its come up in other repo - wondering about checking in lock files

@mmorel-35 was saying that its discouraged due to platform differences - im trying to understand how reproducibility is ensured without doing so (and i guess similar with the added security that using hashes provides)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

right now the lock files just aren't very stable, and across platforms i think they can differ. i think soon it will be reasonable to check them in. but bazel is otherwise using a similar algorithm to go so the dep resolution is reproducible, with the only exception I know of being if upstream yanks a version it can break (but that's not the worst thing)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if upstream yanks a version it can break

but without hashes surely if either the dep pull was subject to MITM or upstream republished to same version (perhaps the registry doesnt allow that and that is the protection - similar to eg pypi) then you would have no way o knowing

dont want to block, just trying to understand the state of the art wrt bazel deps

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yea MITM for sure. It would have to MITM the registry in general. Theoretically it's not allowed to republish the same version (but technically possible)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm yeah - wondering if the registry itself should be signed a la apt etc

@keith keith requested a review from phlax June 18, 2024 18:57
@phlax
Copy link
Copy Markdown
Member

phlax commented Jun 18, 2024

cc @adisuissa for signoff

@mmorel-35
Copy link
Copy Markdown
Contributor

Hi @adisuissa ,
Would like to review when you find the time ;) ?

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.

4 participants