search

bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
387 stars 180 forks source link

Add option to include runfiles files in the `select_file` search #467

Closed kpark-hrp closed 10 months ago

kpark-hrp commented 11 months ago

Adds include_runfiles boolean argument to the select_file rule.

If set to True, runfiles files of the target is also searched for the matching subpath file.

resolves #466

google-cla[bot] commented 11 months ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

kpark-hrp commented 11 months ago

Hello team, @Wyverald @brandjon @tetromino

Can I get a review?

Wyverald commented 11 months ago

@tetromino is on leave until next year. I'm rather unfamiliar with this part of the code, so I'll leave the review to @brandjon.

kpark-hrp commented 11 months ago

I see. @brandjon Can you take a look at this small PR?

The default behavior doesn't change. It just adds an option.

kpark-hrp commented 11 months ago

@Wyverald Hey, is it possible for you to review or let @brandjon know personally?

comius commented 11 months ago

Skylib is one of the core libraries at least I think so. I believe it should have a higher bar to introduce new features. If there were more users requesting such a feature, I'd be more open to introducing it.

Also, PR lacks testing and support for data runfiles or symlinks that runfiles can have.

kpark-hrp commented 11 months ago
  1. Bazel doc says avoid using data_runfiles. https://bazel.build/rules/lib/providers/DefaultInfo#data_runfiles
  2. Can symlink support be added later if there is a request for it. I am currently only searching from the files parameter of the runfiles https://bazel.build/rules/lib/builtins/runfiles.html

@comius I can add a test or two for along with this PR. Would that be enough?

It would be nice to have this merged to the upstream, so I don't have to maintain a fork or patch of this skylib rule. Also, this doesn't change the default behavior of this rule, so I believe it's rather harmless.

comius commented 10 months ago

I have a strong argument against this change. This flattens a depset, and we should avoid that if possible. Specially in rules coming with skylib.