extract_metadata_from_bazel_xml.py changes from #25272

[jtattermusch]

The extract_metadata_from_bazel_xml.py changes from #25272,
but with some extra fixes and improvements (see individual changes commit-by-commit).

The original PR is too big to review properly (and several things are being done at once so it's really hard to make sense of things). After separating the code, I was able to spot some issues and I fixed them.

[jtattermusch]

I don't expect problems there but ran adhoc distribtests anyway, just to be sure:
https://fusion2.corp.google.com/invocations/462ed1f5-b564-4b94-bf0f-7e8bbafb4836

[jtattermusch]

@lidizheng if you're happy with the contents of this PR, feel free to approve and merge.
After that you can rebase #25272 (or create a fresh PR from the rebase) which will hopefully be much easier to review and we won't be distracted by tons of modifications needed both in the extract_metadata_from_bazel_xml.py script, as well as the generated files (the diffs are large).

[lidizheng]

The review is based on lidizheng#2. The changes are:

1. Remove xds-proto related stuff;
2. Make transitive_deps a set instead of list;
3. Add a transitive_public_deps intermediate variable.

First 2 items looks good, item 3 I think it can be removed. In #25272, that behavior has been tested thoroughly by CI. The transitive_public_deps is exactly exclude_deps from an algorithm perspective, but requires extra computation. If we add back the exclude_deps.add(dep) line, then we don't need to traverse the transitive deps again.

Thanks for creating this separate PR, and pushing this effort forward.

[lidizheng]

Emm... this is equivalent to exclude_deps. Why should we calculate it again?

[jtattermusch]

So the previous code "happened to work" but it didn't seem to me that was intentional.

This is addressing the concern I raised in https://github.com/grpc/grpc/pull/25272/files#r590297759 and I didn't feel it was addressed properly by you. The change I made is best seen when you look at commit fbcce34

The issue was that in the original version of the code, you were adding "transitive deps of our public dep" to exclude_deps. (that's correct) But there was an extra exclude_deps.add(dep) line that was also adding the public dependency itself to exclude_deps - but inconsistently with collapsed_deps and transitive_deps, it's adding dep (= the bazel lable) instead of the public depdency name (bazel_label_to_dep_name[dep]).

Later you are excluding the exclude_deps from collapsed_deps - but in this case you only want to remove the intermediate deps, not the public deps (because they are actual dependencies that need to be visible in the result; public deps should only be excluded when collapsing sources and headers). But as you can see, you are filtering filter(lambda x: x not in exclude_deps, collapsed_deps) which from the code looks like your intent is to removing the public deps as well (which doesn't make sense). It happens to work because the collapsed_deps contain converted names for public deps (using bazel_label_to_dep_name), while exclude_deps contain the bazel labels for the public deps. So the logic is basically wrong, but it happens to work due to exclude_deps containing bazel label instead of dependency name by mistake.

This wasn't easy to spot, but as I was looking at the algorithm, it didn't make sense (it should not work as I pointed out previously in my comments). But after experimenting a little bit I found out the bazel_label_to_dep_name[dep] vs dep difference.

[lidizheng]

If we make transitive_deps a set, it will lost the "build order", isn't that one of the things you asked in the comments?

[jtattermusch]

I don't think so.

1. when I made this change I regenerated the yaml files and there was no difference in the generated files
2. the transitive deps aren't actually exposed anywhere in the resulting yaml file, it's just an internal field (fields like collapsed_deps get sorted, which would cancel any effect of transitive deps being ordered).
3. it doesn't matter since the algorithm is agressively eliminating transitive public dependencies in the output. If something depends on both grpc and gpr, only grpc will end up in the final list of dependencies. So (as you pointed out in previous dicussion), the build order doesn't matter since in all cases where it would matter (e.g. gpr coming before grpc, which would be incorrect), the final collapsed_deps will never contain both libraries that have a dependency relationship between them.

[lidizheng]

I guess I will go ahead and merge it, we can resolve the comments in #25272.

[jtattermusch]

Thanks for the review.

Add item 3. (transitive_public_deps) - the original version "happened to work" due to 2 problems in the original algorithm's logic that cancelled each other out. So I made a fix that made the algorithm conceptually correct - but yeah it generated the same result, that's why the all the tests were passing for you.

[jtattermusch]

So the previous code "happened to work" but it didn't seem to me that was intentional.

This is addressing the concern I raised in https://github.com/grpc/grpc/pull/25272/files#r590297759 and I didn't feel it was addressed properly by you. The change I made is best seen when you look at commit fbcce34

The issue was that in the original version of the code, you were adding "transitive deps of our public dep" to exclude_deps. (that's correct) But there was an extra exclude_deps.add(dep) line that was also adding the public dependency itself to exclude_deps - but inconsistently with collapsed_deps and transitive_deps, it's adding dep (= the bazel lable) instead of the public depdency name (bazel_label_to_dep_name[dep]).

Later you are excluding the exclude_deps from collapsed_deps - but in this case you only want to remove the intermediate deps, not the public deps (because they are actual dependencies that need to be visible in the result; public deps should only be excluded when collapsing sources and headers). But as you can see, you are filtering filter(lambda x: x not in exclude_deps, collapsed_deps) which from the code looks like your intent is to removing the public deps as well (which doesn't make sense). It happens to work because the collapsed_deps contain converted names for public deps (using bazel_label_to_dep_name), while exclude_deps contain the bazel labels for the public deps. So the logic is basically wrong, but it happens to work due to exclude_deps containing bazel label instead of dependency name by mistake.

This wasn't easy to spot, but as I was looking at the algorithm, it didn't make sense (it should not work as I pointed out previously in my comments). But after experimenting a little bit I found out the bazel_label_to_dep_name[dep] vs dep difference.

[jtattermusch]

I don't think so.

1. when I made this change I regenerated the yaml files and there was no difference in the generated files
2. the transitive deps aren't actually exposed anywhere in the resulting yaml file, it's just an internal field (fields like collapsed_deps get sorted, which would cancel any effect of transitive deps being ordered).
3. it doesn't matter since the algorithm is agressively eliminating transitive public dependencies in the output. If something depends on both grpc and gpr, only grpc will end up in the final list of dependencies. So (as you pointed out in previous dicussion), the build order doesn't matter since in all cases where it would matter (e.g. gpr coming before grpc, which would be incorrect), the final collapsed_deps will never contain both libraries that have a dependency relationship between them.