Skip to content
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

Convert BUILD.$name-$version.bazel from tera to serde_starlark #1743

Merged
merged 1 commit into from
Jan 4, 2023

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Jan 3, 2023

Context: see #1734.

This PR converts all of crate_build_file.j2, partials/crate/*.j2 and partials/starlark/*.j2 to serde_starlark.

Remarks

  • I kept the output of bazel run //crate_universe/3rdparty:crates_vendor out of this PR to keep GitHub's code review interface usable. You can find the output generated code in 44c264a.

  • I tried to keep the conversion 1-to-1 to the largest extent possible. As an example, build_script.j2 duplicated most of the content of common_attrs.j2 even though it probably could have used that template via include, and I have mirrored the same here by not using #[serde(flatten)] common: CommonAttrs inside of CargoBuildScript. There is probably plenty of opportunity for cleaning up things in followup PRs.

  • I came across several bugs in the existing code. Again, I mostly stayed away from these to keep the PR bounded in scope. For example the fact that rustc_flags is stored as a BTreeSet<String> is not correct; compiler flags are ordered and sorting them is not correct. For example -C debuginfo=2...

    pub rustc_flags: SelectStringList,

  • One bug that I fixed because in the new implementation it was easier to fix than to not fix, is that all existing uses of selectable_list.j2 and selectable_dict.j2 were broken because they all had the same failure mode as crate_universe does not consolidate configurations that match the same set of platforms. #1417. The generated code using selects.with_or could end up generating a map with duplicate keys if multiple distinct Cargo cfg expressions mapped to the same subset of supported platform triples. My implementation follows the same approach as Fix issue #1417 - Group deps by platform triple rather than cfg(...) string. #1647, but that PR only applied it to deps and aliases, not e.g. rustc_env and everything else.

  • Anecdotally the new implementation has strong "if it compiles, it works" property. I mostly typed out the implementation of this PR top to bottom without running crates_vendor, and as soon as the whole thing was written and the compiler was okay with it, it was generating the output I wanted. I have not done much work in the templates in the past but I do not imagine they have this property.

Remaining work

  • select_with_or in private/selects.bzl is no longer used by the code generator's output. I had to keep it in this PR because all the generated BUILD files still include calls to it. It can be deleted after all those are regenerated.

  • One of the tera templates still uses the old serialization of SelectList so the starlark serialization for it isn't a normal Serialize impl but instead an associated function. This causes there to be a bunch of serialize_with = "SelectList::serialize_starlark" attributes needed. This can be cleaned up after the last tera template is converted.

  • A lot of the contents of crate::context can be cleaned up after they're no longer used by tera. Tera needs those data structures to be serializable, whereas with serde_starlark we're not serializing anything from crate::context, only from crate::utils::starlark. For now I've just added a bunch of serde(skip) attributes in crate::context on all the parts tera is no longer using. Edit: apparently we still need to serialize all the fields in Context so that they end up in cargo-bazel-lock.json.

  • There are 2 remaining tera templates after this PR, module_bzl.j2 and vendor_module.j2.

@dtolnay dtolnay force-pushed the starlark branch 5 times, most recently from 08e6bf5 to 5a39df2 Compare January 3, 2023 05:02
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for putting it together! Feel free to update the vendored files (as a separate commit in this PR)

crate_universe/private/selects.bzl Outdated Show resolved Hide resolved
crate_universe/src/utils/starlark/select.rs Show resolved Hide resolved
@dtolnay dtolnay force-pushed the starlark branch 2 times, most recently from d5a493e to bba5b8b Compare January 3, 2023 19:57
@dtolnay
Copy link
Contributor Author

dtolnay commented Jan 3, 2023

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Ship it! Thanks!

@dtolnay
Copy link
Contributor Author

dtolnay commented Jan 4, 2023

Clean rebase.

@illicitonion illicitonion merged commit e7c8a97 into bazelbuild:main Jan 4, 2023
@dtolnay dtolnay deleted the starlark branch January 4, 2023 16:43
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