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 includes attr to automate $PERL5LIB #62

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

lalten
Copy link
Contributor

@lalten lalten commented Jul 25, 2024

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 lalten requested a review from skeletonkey as a code owner July 25, 2024 22:40
@lalten
Copy link
Contributor Author

lalten commented Aug 4, 2024

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

@polasek
Copy link

polasek commented Aug 7, 2024

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

lalten commented Aug 7, 2024

an includes attribute to perl_library and perl_binary, 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
Copy link
Contributor Author

lalten commented Aug 7, 2024

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 lalten changed the title Automate PERL5LIB Add includes attr to automate $PERL5LIB Aug 8, 2024
@lalten
Copy link
Contributor Author

lalten commented Aug 10, 2024

@skeletonkey friendly ping

Wyverald pushed a commit to bazelbuild/bazel-central-registry that referenced this pull request Aug 19, 2024
This is latest rules_perl plus
* bazel-contrib/rules_perl#62
* bazel-contrib/rules_perl#65
* bazel-contrib/rules_perl#66

Obviously it would be nice to get a review / merge this upstream. If
that still happens we can add a new (0.2.4 ?) release without patches in
the BCR.

The patch enables external rules_perl Bazel repos to work under Bzlmod.
This is needed for https://github.com/lalten/rules_cpan to work out of
the box

needs the `unstable source url` label
@saw235
Copy link

saw235 commented Aug 24, 2024

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

lalten commented Aug 24, 2024

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 #65 helps?

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

@skeletonkey skeletonkey merged commit 9cbb859 into bazel-contrib:main Aug 29, 2024
2 checks passed
@lalten
Copy link
Contributor Author

lalten commented Aug 29, 2024

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 :)

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.

4 participants