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

Normalize path returned by get_coursier_cache_or_default in _relativize_and_symlink_file_in_coursier_cache #825

Conversation

anthonyjpratti
Copy link
Contributor

Fixes #824.

Ideally, this would use paths.normalize in skylib, but that would require changes to how rules_jvm_external setups up dependencies.

@shs96c
Copy link
Collaborator

shs96c commented Jan 12, 2023

FYI, skylib is pulled in as a dependency via the repositories.bzl file. It's safe to rely on it. Would you prefer to use that?

@anthonyjpratti
Copy link
Contributor Author

FYI, skylib is pulled in as a dependency via the repositories.bzl file. It's safe to rely on it. Would you prefer to use that?

So that was my first attempt, but I ran into the following issue. In rules_jvm_external_deps func, there is a bazel_skylib http archive and a maven_install; since maven_install relies on coursier.bzl and bazel_skylib isn't defined until rules_jvm_external_setup is invoked, skylib fails to be loaded in coursier.bzl. Since rules_jvm_external_deps and rules_jvm_external_setup are part of the public api for rules_jvm_external rules, I didn't want to modify that.

I'm open to recommendations on how to import skylib without breaking backwards compatibility. If there aren't concerns about backwards compatibility, then I do have a path forward for relying on skylib, but that's y'alls call.

@shs96c
Copy link
Collaborator

shs96c commented Jan 13, 2023

How frustrating! Well then, this will be excellent :) Thank you for the patch, and the explanation!

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shs96c shs96c merged commit 52edfdd into bazel-contrib:master Jan 13, 2023
@anthonyjpratti
Copy link
Contributor Author

How frustrating! Well then, this will be excellent :) Thank you for the patch, and the explanation!

I found it to be a great learning experience! I earnestly love when things break in ways I don't expect! :)

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.

bazel run @unpinned_maven//:pin will fail if HOME as a trailing slash
2 participants