search

bazelbuild / rules_android_ndk

Apache License 2.0
31 stars 15 forks source link

Build with incompatible_disallow_empty_glob #37

Closed limdor closed 1 year ago

limdor commented 1 year ago

In order to flip the flag, all downstream projects should be adapted. However, it is hard to fix them all if there are constant regressions. Adding it to the CI will ensure that once the project can build with incompatible_disallow_empty_glob it can keep building like that. See: bazelbuild/bazel#15327

cpsauer commented 1 year ago

Came across this while trying to flip the same :) Agreed, but @limdor, I think this is probably unlikely to get merged until it builds cleanly with the flag--the empty globs are removed. Would you be down to take a shot at fixing the globs, adding that to this PR?

limdor commented 1 year ago

I can take a shot on fixing the globs only if the maintainers commit to review the PRs in a timely manner including this one. In several repos I had to fix it several times because it was not enforced in the CI. Also depend on how to fix it. Making the current behavior explicit is trivial but some maintainers are not happy to see that their gobs resolves to empty lists. My experience when trying to flip this flag is that in general people want a hidden empty blob or a fix that makes the glob not empty but not making it explicitly allowed empty. That is way before doing more changes I would like first an statement from maintainers, in the end are the ones who have to approve the PRs and maintain the code later.

limdor commented 1 year ago

@cpsauer I took a look and setting explicit the current behavior was quite trivial. Now some maintainer should say if we can merge it like that or not.

limdor commented 1 year ago

In general this looks fine, but FWIW, setting allow_empty = True in various glob()s while activating incompatible_disallow_empty_glob only masks the root cause of the problem: that several globs in the NDK directory return nothing. If anything, this design pattern could turn into one of those silent-but-deadly bugs that are difficult to debug.

I also noticed that this PR did not have a reviewer assigned (currently ahumesky is just listed as the suggested reviewer). In the future, please assign myself or ahumesky to the PR if you want a response.

I agree partially. It masks the problem in the sense that you can consider this as a lost opportunity to take a look but on the other side now is not transparent because you only have to look for the places where allowed empty is set to true. Before the change you could not easily know which ones resolve to empty.