Replaced m4, flex, and bison targets with jmillikin's rules.

[abrisco]

This updates the rules to use rules_m4, rules_bison, and rules_flex to eliminate deficiencies (most notably the use of genrule in my case) which make building the rules hard on other platforms like Windows.

[QuantamHD]

Can you move these to dependency_support.bzl

[abrisco]

I would love to but unfortunately I'd need to load these rules first, which happens in dependency_support.bzl 😞. I can do so if you can suggest where I should load the rules in the first place

[QuantamHD]

This seems like a backwards incompatible change. Would this cause errors for downstream users?

[abrisco]

To my knowledge this rule is only used internally so the change should not have an impact. Their existence in a private directory was to discourage users from loading them (as buildifier will complain with bzl-visibility).

[abrisco]

Also, the Bazel Test Log from the last build is not visible to me. It's asking me to log in.

[QuantamHD]

Also, the Bazel Test Log from the last build is not visible to me. It's asking me to log in.

I think since the build failed during repo fetch no uuid for the test was ever uploaded. It seems like an edge case of the way we upload to build buddy

[abrisco]

@QuantamHD finally got CI passing, this change is now ready for another review!

[QuantamHD]

So I still have a concern with this change. Lots of downstream repos import bazel_rules_hdl into their own repo and M4 and others not being the dependency support means they won't be available in downstream repos.

[abrisco]

So I still have a concern with this change. Lots of downstream repos import bazel_rules_hdl into their own repo and M4 and others not being the dependency support means they won't be available in downstream repos.

@QuantamHD It seemed to me that dependency_support was more implementation detail. It wouldn't be hard to leave the repositories but if rules_hdl is not using them, it seems the correct thing to delete them. Is everything in dependency_support considered to be a public interface? Or does it primarily exist to provide the necessary dependencies to power the rules?

edit:

An example of a similar change would be if rules_rust, which defines a tinyjson repository for the Rust compiler process wrapper, were to change or delete this dependency. While it's going to be defined for all consumers of the rules, it's defined by rules_rust to provide the necessary behavior for compiling Rust code and any users also using this library would need to either add their own pins or account for the changes the next time they bump rules_rust. So the being said, the change is extremely contingent on whether or not these dependencies are considered public interface. And if so, some documentation like @rules_rust//:ARCHITECTURE.md would be awesome so contributors such as myself know how to make changes while keeping everyone happy 😄

[abrisco]

poke @QuantamHD 😄

[QuantamHD]

My concern is more that anything that appears in the transitive dependencies of verilator, openroad, etc. that have rules that depend on the correct execution of those rules then those need to be declared in dependency_support otherwise they won't build when a user "imports" bazel_rules_hdl.

bazel_rules_hdl is more akin to rules_rust than to a standalone repo even though it does more than most rules repos.

By default users are expected to call a function that will run depedency_support() in their repo. At the end of the day the thing I'm worried about is whether or not the rules for various things will still work in other bazel workspaces

[abrisco]

My concern is more that anything that appears in the transitive dependencies of verilator, openroad, etc. that have rules that depend on the correct execution of those rules then those need to be declared in dependency_support otherwise they won't build when a user "imports" bazel_rules_hdl.

bazel_rules_hdl is more akin to rules_rust than to a standalone repo even though it does more than most rules repos.

By default users are expected to call a function that will run depedency_support() in their repo. At the end of the day the thing I'm worried about is whether or not the rules for various things will still work in other bazel workspaces

load("@rules_hdl//dependency_support:dependency_support.bzl", rules_hdl_dependency_support = "dependency_support")
rules_hdl_dependency_support()

load("@rules_hdl//:init.bzl", rules_hdl_init = "init")
rules_hdl_init()

@QuantamHD Ah ok. I think for this change CI should cover that concern unless you are aware of existing gaps. For Verilator in particular the following test should cover the ability to compile Verilator and use it to generate outputs:

bazel_rules_hdl/verilator/tests/BUILD

Lines 62 to 71 in c2ef195

	cc_test(
	name = "load_and_count_test",
	srcs = [
	"load_and_count_test.cc",
	],
	deps = [
	":load_and_count_verilator",
	"@com_google_googletest//:gtest_main",
	],
	)

Dependencies of this target would either fail to build or the unit test would fail if there was a major issue in the code. I feel for this change that the impact is covered by similarly existing tests. Additionally, this works in my own workspace where I have the exact lines of code you referenced and heavy use of Verilator and Verilog.

[QuantamHD]

But that's within this WORKSPACE file. If bazel_rules was instantiated in another workspace my expectation is that that test would fail because the rules required to build openroad or one of the other binaries would fail to find an m4 dependency

[QuantamHD]

See https://github.com/google/xls/blob/c71db4e7ce02a9c46a7e87c7ff1087dbd2b82370/dependency_support/rules_hdl/workspace.bzl#L45

[abrisco]

But that's within this WORKSPACE file. If bazel_rules was instantiated in another workspace my expectation is that that test would fail because the rules required to build openroad or one of the other binaries would fail to find an m4 dependency

@QuantamHD As long as the contract is folks load the following somewhere in their workspace and all the external transitive dependencies for rules_hdl come from the same pin, testing within the same workspace should be sufficient

load("@rules_hdl//dependency_support:dependency_support.bzl", rules_hdl_dependency_support = "dependency_support")
rules_hdl_dependency_support()

load("@rules_hdl//:init.bzl", rules_hdl_init = "init")
rules_hdl_init()

See https://github.com/google/xls/blob/c71db4e7ce02a9c46a7e87c7ff1087dbd2b82370/dependency_support/rules_hdl/workspace.bzl#L45

Here we see the dependencies being initialized from the repo, not independently.
https://github.com/google/xls/blob/c71db4e7ce02a9c46a7e87c7ff1087dbd2b82370/dependency_support/initialize_external.bzl#L37

So updating the pin of rules_hdl will pull in this change holistically and avoid a breakage in any transitive dependencies.

[QuantamHD]

Okay I see. LGTM!

I missed the fact that those were in the init function the first time around. My bad.

[abrisco]

Okay I see. LGTM!

I missed the fact that those were in the init function the first time around. My bad.

All good, thank you for the review!