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 workspace_prefix_for_pinned_deps #724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tgeng
Copy link

@tgeng tgeng commented Aug 2, 2022

When artifacts are pinned, maven_install would create individual
external repos for each dependency. The name of such an external repo is
simply the maven coordinate with all punctuations replaced by _. This
can cause name conflicts for code bases that are already using
http_archive to download thousands of dependencies through some other
mechanism but would like to migrate to use maven_install.

This PR tries to fix this by specifying a prefix for intermediate
external repos so that name collision becomes manageable.

@tgeng tgeng requested a review from cheister as a code owner August 2, 2022 17:03
@google-cla
Copy link

google-cla bot commented Aug 2, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@tgeng
Copy link
Author

tgeng commented Aug 10, 2022

Hi @cheister , can you take a look at this change? Thanks!

@tgeng
Copy link
Author

tgeng commented Aug 22, 2022

Hi @shs96c can you take a look at this?

@shs96c
Copy link
Collaborator

shs96c commented Aug 30, 2022

Apologies for the slow review on this. Here now!

First comment: if you want a prefix, then I'd suggest defaulting to the repository rule's name. Secondly, if the versions pointed to are the same (down to the sha256), then it shouldn't matter which one is being used or where it's being pulled from. If this is the case, then it would be better to use a maybe to load the http_file, rather than a prefix.

Would that approach work for you?

@tgeng
Copy link
Author

tgeng commented Sep 1, 2022

Apologies for the slow review on this. Here now!

First comment: if you want a prefix, then I'd suggest defaulting to the repository rule's name. Secondly, if the versions pointed to are the same (down to the sha256), then it shouldn't matter which one is being used or where it's being pulled from. If this is the case, then it would be better to use a maybe to load the http_file, rather than a prefix.

Would that approach work for you?

Hi, maybe would not solve our problem because in our current code base we are creating external repos with a different mechanism, which could result in artifacts that are of the same version but loaded from different sources which are built differently (and hence may have different SHA). My purpose of this PR is to allow easier adoption of maven_install so that it will not interfere with existing code.

Defaulting the prefix to repo name would be a breaking change for existing users. Also, moving forward, with our use case (and typical use cases of others), we actually want to share the same intermediate artifacts across multiple pinned maven install. So it seems defaulting to "" is still the desirable behavior for most use cases.

BTW, regardless of this change, it does seem that without a prefix, the same http_file could be registered multiple times with multiple maven_install. So it seems all http_file calls in defs.bzl should be wrapped inside maybe. I can also update this PR to include such change if desired.

What do you think?

@tgeng
Copy link
Author

tgeng commented Sep 6, 2022

gentile ping @shs96c

When artifacts are pinned, `maven_install` would create individual
external repos for each dependency. The name of such an external repo is
simply the maven coordinate with all punctuation replaced by `_`. This
can cause name conflicts for code bases that are already using
`http_archive` to download thousands of dependencies through some other
mechanism but would like to migrate to use `maven_install`.

This PR tries to fix this by specifying a prefix for intermediate
external repos so that name collision becomes manageable.
@tgeng
Copy link
Author

tgeng commented Nov 7, 2022

Any updates? @shs96c

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.

None yet

2 participants