Add bazel build system support for simple_switch_grpc#1401
Conversation
Signed-off-by: c8ef <c8ef@outlook.com>
Signed-off-by: c8ef <c8ef@outlook.com>
Signed-off-by: c8ef <c8ef@outlook.com>
Signed-off-by: c8ef <c8ef@outlook.com>
|
cc @matthewtlam @qobilidop for review. |
There was a problem hiding this comment.
Is this file needed? I would remove it
There was a problem hiding this comment.
Yes, it is necessary for the copyright check. We cannot include the copyright notice in the .bazelversion file.
There was a problem hiding this comment.
@matthewtlam If .bazelversion files have an explicit comment syntax that they support (e.g. everything after a '#' character on a line is a comment), then it is usually preferred to have the copyright and license notice in comments in the file itself.
If .bazelversion files do not suport comments, then the REUSE tool that is now being used in behavioral-model (and soon hopefully all p4lang repositories) requires that there be a separate file with a suffix of ".license" in its name that contains the copyright and license info. There is also a way to put this info in a different separate file, but the ".license" file is good if there are only a few files that don't support comment syntax.
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| module( | ||
| name = "behavioral-model", |
There was a problem hiding this comment.
I don't have a strong opinion on this, but is using this different from the project name on GitHub?
| bazel_dep(name = "rules_cc", version = "0.2.17") | ||
| bazel_dep(name = "xxhash", version = "0.8.3.bcr.1") | ||
|
|
||
| bazel_dep(name = "googletest", version = "1.17.0.bcr.2", dev_dependency = True) |
There was a problem hiding this comment.
I see you put everything in alphabetical order. Is there an obvious reason why this one isn't either?
There was a problem hiding this comment.
Buildifier requires dev dependencies to be listed separately.
| # SPDX-FileCopyrightText: 2026 Yuao Ma | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
There was a problem hiding this comment.
It is apart of Google standard to have the following https://github.com/p4lang/p4c/blob/main/BUILD.bazel#L8C1-L20C27
There was a problem hiding this comment.
I just realized that there isn't a root-level license file for bmv2. Given this, I'm not sure if simply adding an Apache 2.0 notice is the best approach. Love to hear your thoughts on this @jafingerhut.
There was a problem hiding this comment.
I will add a root-level LICENSE file that is a symbol link to LICENSES/Apache-2.0.txt soon.
There was a problem hiding this comment.
Done a few minutes ago when I merged this PR: #1403
If you update this PR's branch to the latest master/main version, the LICENSE file should be there.
| # SPDX-FileCopyrightText: 2026 Yuao Ma | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
There was a problem hiding this comment.
Will need to add a package statement in all .bazel files for example: https://github.com/google/p4-infra/blob/903bebb041cc95b8aadb3e4ea8a216a0081de584/p4_pdpi/BUILD.bazel#L25C1-L28C2
There was a problem hiding this comment.
Should I still add package level visibility even though I have explicitly added visibility = ["//visibility:public"] for all cc_library?
| load("@bazel_skylib//rules:expand_template.bzl", "expand_template") | ||
| load("@rules_cc//cc:cc_library.bzl", "cc_library") | ||
|
|
||
| exports_files(["VERSION"]) |
There was a problem hiding this comment.
Could you say a little bit of why we export the version in this file specifically?
There was a problem hiding this comment.
Because I want to use the content of the VERSION file directly to parse the version number. It's a bit hacky, but it will make release easier.
| "src/*.h", | ||
| ]), | ||
| hdrs = ["bm/PI/pi.h"], | ||
| includes = ["."], |
There was a problem hiding this comment.
Might be okay for now since we are adding support for Bazel in legacy code, but is generally discouraged in Bazel projects because of
Header Collisions: If multiple libraries use includes = ["."], and both have a file named utils.h, it can lead to compiler confusion and hard-to-debug header resolution errors.
Explicit is Better: Bazel prefers absolute paths relative to the workspace root (e.g., #include "src/parser/parser.h") because it makes it immediately clear where a header is coming from.
Perhaps try removing them and seeing if the tests still pass
There was a problem hiding this comment.
I tried this approach, but it didn't work. After investigating, I found that bmv2 includes BM-related headers using angle brackets (< >), which means the paths must be passed explicitly using -I. I attempted to use strip_include_prefix, but it passes the paths to the compiler via -iquote, which doesn't resolve angle bracket includes. Simply removing these includes is not an option.
ref: https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html
Directories specified with -iquote apply only to the quote form of the directive, #include "file". Directories specified with -I, -isystem, or -idirafter apply to lookup for both the #include "file" and #include <file> directives.
|
|
||
| load("@rules_cc//cc:cc_test.bzl", "cc_test") | ||
|
|
||
| _COMMON_SRCS = [ |
There was a problem hiding this comment.
Is there a particular reason why these are not apart of the _COMMON_DEPS?
There was a problem hiding this comment.
_COMMON_SRCS is a list of source files, whereas _COMMON_DEPS is a list of local or remote dependencies.
| "test_optional", | ||
| ] | ||
|
|
||
| [ |
There was a problem hiding this comment.
Overall Bazel strongly discourages adding unnecessary dependencies and explicit library dependencies, but this is okay since I feel like all the test require all the exact same dependencies
There was a problem hiding this comment.
Yeah, it seems like the same dependencies are being used but different scenarios are being tested.
Signed-off-by: c8ef <c8ef@outlook.com>
This reverts commit f4bc3c7. Signed-off-by: c8ef <c8ef@outlook.com>
3 participants
Uh oh!
There was an error while loading. Please reload this page.