search

bazelbuild / rules_platform

Apache License 2.0
8 stars 4 forks source link

`platform_data` required executable targets #10

Open tpudlik opened 5 months ago

tpudlik commented 5 months ago

This came up in a code review discussion for an (accidental) re-implementation of platform_data: the target in platform_data is required to be executable = True. I thought the reason it's called platform_data and not platform_binary is specifically because the target doesn't have to be created by a binary rule, but can be any kind of data!

It would be nice to relax this restriction.

tpudlik commented 5 months ago

@katre @aranguyen FYI

afoxley commented 5 months ago

It looks like the executable requirement is just to setup a symlink in the parent build dir (which is nice, since otherwise bazel sticks it in a hard to find dynamic named build dir). So maybe it could only set that symlink only if executable is set?

katre commented 5 months ago

Unfortunately, the executable parameter to rule is set for the rule, and can't be set during analysis.

We could in theory have a macro that redirects to platform_data or platform_binary based on an executable attribute, and get the same effect with a bit more work, but then we'd need a sensible default (since the macro wouldn't be able to guess whether the target is an executable or not).

tpudlik commented 5 months ago

Would it make sense to have two separate rules, platform_data and platform_binary, differing only in the executable parameter?

I actually think platform_binary may be a more self-explanatory name to people who use this rule to wrap binary targets. And if the target is required to be a binary (executable) anyway, there's no loss of generality in replacing the more generic term "data".