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

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

kpark-hrp
Copy link

@kpark-hrp kpark-hrp commented Oct 9, 2023

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
Copy link

google-cla bot commented Oct 9, 2023

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 kpark-hrp marked this pull request as ready for review October 9, 2023 21:06
@kpark-hrp
Copy link
Author

Hello team, @Wyverald @brandjon @tetromino

Can I get a review?

@Wyverald
Copy link
Member

@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
Copy link
Author

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
Copy link
Author

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

@comius
Copy link
Collaborator

comius commented Oct 13, 2023

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 kpark-hrp changed the title Add option to include runfiles in the select_file search Add option to include runfiles files in the select_file search Oct 13, 2023
@kpark-hrp
Copy link
Author

kpark-hrp commented Oct 13, 2023

  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
Copy link
Collaborator

comius commented Oct 17, 2023

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.

@comius comius closed this Oct 31, 2023
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.

Search runfiles in select_file rule
3 participants