Packaging/RequireHardcodingLib: Warning when absolute path was used

[olleolleolle]

Offenses:

spec/generators/paper_trail/install_generator_spec.rb:5:1: C: Packaging/RequireHardcodingLib: Avoid using require with relative path to lib/. Use require with absolute path instead.
require File.expand_path("../../../lib/generators/paper_trail/install/install_generator", __dir__)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This warning happened.

But, File.expand_path was used, so it is an absolute path.

Perhaps the check can include knowing what File.expand_path does to a path?

[utkarsh2102]

Hi @olleolleolle 👋🏻

So the issue is using such calls from the spec/ to lib/ - which is across components. So when we perform the system wide testing, with lib/ code installed in its usual way, we get a LoadError. See here: https://packaging.rubystyle.guide/#using-require-relative-from-test-to-lib-rationale/

Does this make sense? Should I/we elaborate more? Or should we improve the documentation and make it more verbose?

[deivid-rodriguez]

Hi! What I'm noticing is that this "cop" is not really about require but about any code assuming specific relativeness between spec/ and lib/, isn't it? So this cop could potentially detect usages of File.expand_path("../../../lib/generators/paper_trail/install/install_generator", __dir__) anywhere.

[utkarsh2102]

Hi @deivid-rodriguez,

What I'm noticing is that this "cop" is not really about require but about any code assuming specific relativeness between spec/ and lib/, isn't it?

I am not sure what that means?
This cop only checks the require calls, here's its AST:

rubocop-packaging/lib/rubocop/cop/packaging/require_hardcoding_lib.rb

Lines 46 to 52 in 9eeb5a8

	def_node_matcher :require?, <<~PATTERN
	{(send nil? :require (str #falls_in_lib?))
	(send nil? :require (send (const nil? :File) :expand_path (str #falls_in_lib?) (send nil? :__dir__)))
	(send nil? :require (send (const nil? :File) :expand_path (str #falls_in_lib_using_file?) (str _)))
	(send nil? :require (send (send (const nil? :File) :dirname {(str _) (send nil? _)}) :+ (str #falls_in_lib_with_file_dirname_plus_str?)))
	(send nil? :require (dstr (begin (send (const nil? :File) :dirname {(str _) (send nil? _)})) (str #falls_in_lib_with_file_dirname_plus_str?)))}
	PATTERN

[deivid-rodriguez]

Yeah, I know! I'm just saying that it doesn't need to be!

If I have this code in my spec/generators/paper_trail/install_generator_spec.rb file:

foo = File.expand_path("../../../lib/generators/paper_trail/install/install_generator", __dir__)

Then it'd be useful for you debian packagers that a cop alerts me that this is not friendly for you. Right?

[olleolleolle]

I want to take a moment to thank you both, @deivid-rodriguez and @utkarsh2102 for taking my short description describing it so well.

[utkarsh2102]

Yeah, I know! I'm just saying that it doesn't need to be!

Oooh, that's a great suggestion, indeed!

If I have this code in my spec/generators/paper_trail/install_generator_spec.rb file:

foo = File.expand_path("../../../lib/generators/paper_trail/install/install_generator", __dir__)

AFAIR, this is what happened recently, no? The issues I reported with rubygems and it boiled down to such lines of code? Or was it something different?

Then it'd be useful for you debian packagers that a cop alerts me that this is not friendly for you. Right?

Indeed, I think this is a great idea! We need to:

• Tweak the AST.
• Add more tests.
• Change the cop name.

[utkarsh2102]

Hey, @samyak-jn, do you want to give this a shot?
(since you wanted to do something here? :P)

[samyak-jn]

Hey @utkarsh2102,
Thanks, I am happy to give it a try. 🌮
I have taken pointers from your last comment, will ping you if stuck! :P

[deivid-rodriguez]

AFAIR, this is what happened recently, no? The issues I reported with rubygems and it boiled down to such lines of code? Or was it something different?

Correct, that's exactly how rubygems tests broke in debian last time 👍.