search

bazel-contrib / rules_perl

Perl rules for Bazel
Apache License 2.0
26 stars 37 forks source link

Add `includes` attr to automate `$PERL5LIB` #62

Closed lalten closed 2 months ago

lalten commented 4 months ago

The current advice for loading perl_libraries is to manually add a PERL5LIB env on perl_binary. This only really works for perl_library dependencies in the same workspace and for external dependencies in WORKSPACE mode. In Bzlmod paths like ../fcgi/lib can't be hard-coded (at least not when the repo isn't always _main but is a library itself).

This PR automates setting the PERL5LIB by means of a new includes attr on the perl_library rule.

This seems to work well in my (very limited) test environment.

(Note: I needed to comment out HAVE_DEV_PTMX to get the perl_xs to compile on my machine 🤷)

lalten commented 3 months ago

This is needed for https://github.com/lalten/rules_cpan to work, would be happy about a review!

polasek commented 3 months ago

@lalten hi, I didn’t see this PR but had to just deal with this recently myself.

One other issue with the existing set up is that it doesn’t really work with runfiles - I am not sure if your PR deals with that or not.

When I took a look at automating this, I added an includes attribute to perl_library and perlbinary, similar to how it’s done in cc* rules. Have you explored this idea at all? It seems more in line with existing rules and explicit. Heuristics can fail especially if anyone tries to do anything non-standard.

lalten commented 3 months ago

an includes attribute to perl_library and perlbinary, similar to how it’s done in cc* rules. Have you explored this idea at all? It seems more in line with existing rules and explicit.

I have not thought about that and I really like the idea. I'll update this PR (when I find some time)

Heuristics can fail especially if anyone tries to do anything non-standard.

I agree, we could just set the includes path default to ["lib", "."]. That should cover most use cases and can be overriden easily

lalten commented 3 months ago

One other issue with the existing set up is that it doesn’t really work with runfiles - I am not sure if your PR deals with that or not.

Can you give an example for me to understand the issue? The library dependencies will certainly be in the runfiles of a target if you add them as deps

lalten commented 3 months ago

@skeletonkey friendly ping

saw235 commented 3 months ago

This doesn't work with genrule. The include path doesn't work correctly if the genrule target is running the binary from elsewhere. Is it possible to make it infer the absolute path instead

lalten commented 3 months ago

This doesn't work with genrule. The include path doesn't work correctly if the genrule target is running the binary from elsewhere. Is it possible to make it infer the absolute path instead

Can you check if https://github.com/bazelbuild/rules_perl/pull/65 helps?

Would be great to have a reproducer, then we can try to make it work

lalten commented 2 months ago

Thanks for merging @skeletonkey! #64 one and #65 are also patched in to the BCR release, would be great if you could check these as well :)