bazelbuild / platforms

Constraint values for specifying platforms and toolchains
Apache License 2.0
110 stars 75 forks source link

platform_data implementation #78

Closed aranguyen closed 1 year ago

aranguyen commented 1 year ago

Implementation of platform_data https://github.com/bazelbuild/platforms/issues/73

gregestren commented 1 year ago

A few comments I'd like you to consider.

I'm a bit worried about the dependency on skylib. It looks like it only exists to allow for bzl_library? I worry about possible circular references if skylib needs something from platforms in the future.

Recapping our discussion with @aiuto today:

We decided to remove the Skylib dependency by removing the bzl_library, then integrate this into the @plaforms repo but not into the @platforms release bundled with Bazel.

aranguyen commented 1 year ago

We still need a BUILD.bazel file in platform_data/, probably with an exports_files call.

Do you have plans for adding tests?

What is the exports_files for?

At this moment I'm not planning on adding explicit tests for it because it is fairly straight forward. Greg also mentioned similarly regarding test in the internal cl review. We can follow up with your platform_test idea when I'm back.

katre commented 1 year ago

exports_files is just to mark that the file is meany to be used outside the package.

We should add tests for all rules, no matter how simple. It doesn't need to block merging this but we should add them at some point (and exclude them from the release).

aranguyen commented 1 year ago

exports_files is just to mark that the file is meany to be used outside the package.

We should add tests for all rules, no matter how simple. It doesn't need to block merging this but we should add them at some point (and exclude them from the release).

Thanks for clarifying that it shouldn't block merging. I'll definitely add tests for this.

For export_files, add defs.bzl?

Also I forgot to move this under experiemental as well. So I will move it under experimental/platform_data