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

fix: filter runfiles with whitespace in their paths #73

Closed
wants to merge 3 commits into from

Conversation

RyanDraves
Copy link
Contributor

Remove runfiles that contain spaces in their filenames, typically seen as gems with poor file naming choices. The --experimental_inprocess_symlink_creation flag is recommended to get around the issue noted in bazelbuild/bazel#4327, but that workaround does not seem to work in bazel build contexts.

@p0deje
Copy link
Member

p0deje commented Feb 1, 2024

Could you please check and fix buildifier errors? https://github.com/bazel-contrib/rules_ruby/actions/runs/7737573441/job/21109769894?pr=73

@p0deje
Copy link
Member

p0deje commented Feb 1, 2024

Thinking out loud, is it really better to silently filter out runfiles with spaces than to fail? Wouldn't it still fail during execution if we drop part of the runfiles and the user would be even more confused as to why it's not there?

@RyanDraves
Copy link
Contributor Author

Thinking out loud, is it really better to silently filter out runfiles with spaces than to fail? Wouldn't it still fail during execution if we drop part of the runfiles and the user would be even more confused as to why it's not there?

That's a good point, I could see that being confusing. Maybe the filtering could be configured some how? My example is https://github.com/jekyll/jekyll-redirect-from/tree/master/spec/fixtures/tags, where there's a test file with a space in it that seems to be part of the distribution of the gem; the file isn't necessary for the gem to be used.

@p0deje
Copy link
Member

p0deje commented Feb 2, 2024

I had the same issue with Steep so I had to soutaro/steep#962. Maybe it would be great to filter runfiles, but what do you think if we also print a message about that? Something like Removing "{src}" from the runfiles because it contains space(s) (https://github.com/bazelbuild/bazel/issues/4327)..

@RyanDraves
Copy link
Contributor Author

I'm having some second thoughts on what the issue really is. I tried to reproduce from the solution we arrived at in #72, and I found some interesting things:

  • jekyll-redirect-from doesn't include the spec/ folder in its .gemspec file
  • This issue only happens if I run bazel run @ruby//:bundle -- install first
  • The solution works for the implementation I originally had in toolchain/gem binary quirks #72, but doesn't work for the better solution we arrived at
  • Getting a fresh cache with bazel clean --expunge was needed after getting into that bad state; if I don't run bazel run @ruby//:bundle -- install first, then I don't need this change

It looks like using bazel run @ruby//:bundle installed things differently and led to those files being left around, but it was bad usage that led to it occurring.

@p0deje
Copy link
Member

p0deje commented Feb 3, 2024

Okay, so I am on the fence whether we need to merge this as-is or do something else. @alexeagle Do you happen to know how other rulesets handle runfiles with spaces?

@alexeagle
Copy link
Collaborator

I don't think this is right, we've never filtered files out of the runfiles in other contexts. I think there needs to be an issue filed on this repo first, showing the existing problem so we can understand what layer the bug is in.

@p0deje
Copy link
Member

p0deje commented Feb 6, 2024

It looks like using bazel run @ruby//:bundle installed things differently and led to those files being left around, but it was bad usage that led to it occurring.

Ok, given the original problem is gone and based on Alex's judgment, I'm going to close this for now. Let's create follow-up issues when somebody runs into the problem.

@p0deje p0deje closed this Feb 6, 2024
@RyanDraves RyanDraves deleted the whitespace-filter branch February 7, 2024 02:15
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.

3 participants