require_relative should never be used with bazel because it reads files from the source tree instead of the sandbox runfiles tree

[noahkawasakigoogle]

I've discovered that the use of require_relative vs require actually reads files from the source tree instead of the files in the bazel sandbox runfiles tree.

Repro: https://github.com/bazel-contrib/rules_ruby/pull/224/files

In this example, you can remove the :spec_helper library from the :add test target and it will still pass even though spec_helper.rb doesnt exist in any of the runfiles. This is because require_relative loads the file directly from the source tree (your local repo) and bypasses Bazel sandbox.

If you change require_relative to require, it will fail to find the spec_helper.rb file until you add back :spec_helper as a dependency.

require always searches files from $LOAD_PATH, and in bazel rules_ruby, the deps, srcs, and bundle repo gems are always on in the list of dirs in $LOAD_PATH.

I think it might be good to add a require_relative static analysis check in rb_library to fail any rb file that has this function used in rules_ruby to avoid this silent/unintentional behavior. The result is that your bazel targets may be unknowingly using files that are not declared as dependencies. Thoughts?

[p0deje]

Ugh, I was not expecting that, but it makes sense judging by the source code of Kernel#require_relative (https://docs.ruby-lang.org/en/3.4/Kernel.html#method-i-require_relative). This is similar to aspect-build/rules_js#362 I guess.

We could solve this by monkey-patching require_relative and first checking that the file exists within the sandboxed tree, then calling the original implementation. Like any other monkey-patch, it might open a can of worms, though.

[noahkawasakigoogle]

Thats true, I think a WARNING log if the target doesnt have local = True during rb_library might be good enough. For local runs it might be fine.

I also found that using require and absolute paths to all ruby files kind of forced me to refactor all require statements to be much more readable and less vague. Kind of like how Java/Kotlin imports are always fully qualified, it just results in more clear code and no guessing where something came from.