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

Use resolved parent for resolving children in normalizer #381

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

Conversation

kmate-ct
Copy link

@kmate-ct kmate-ct commented Feb 6, 2024

A potential solution to #379 by not allowing an evicted version to have effect on selecting a version of its children.

@kmate-ct
Copy link
Author

kmate-ct commented Feb 6, 2024

If we think this is a good addition, I'm willing to add some unit tests!

// fix version of current entry
.updated(node, newItems)
// update all children
.mapValues(_.filter { case (parent, _) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

the intent seems good, but I think this algorithm is quadratic in the number of nodes.

Going from O(N) to O(N^2) here could be a deal breaker for many repos.

I think we could keep it O(N * E) (where E is the average number of edges nodes) if we use the Graph[N, E] data structure which can look up quickly edges that have a given source or destination. The input graph starts with that, but we may need to update that graph as we modify to use this algorithm.

I'm happy to merge something like this with some tests and as long we don't think we have any quadratic behavior.

Thanks for working on this.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this is just a prototype to show what would happen. If we agree that this might be a good direction I could try to make it perform better.

@kmate-ct
Copy link
Author

kmate-ct commented Feb 8, 2024

Update: we consider to change to maven_install from rules_jvm_external for multiple reasons later. I'd need to put this work on hold until we figure that out, as my resources are currently very limited towards this project. Still the patch should work and give the same resolution as using maven_install with pinned version conflict resolution, so whoever needs it could use this version. If I can get more resources to it, I'll return to it, but I cannot guarantee that unfortunately at this time.

@johnynek
Copy link
Collaborator

johnynek commented Feb 8, 2024

Thanks for the update.

BTW: maven_install seems like a good choice. I think the main benefit of Bazel-deps is that it has better scala support. For scala you don't want to run ijar in libraries that have macros or they will fail.

If you don't use scala or leverage any of the customization that Bazel-deps offers I think maven_install could be a fine choice.

@kmate-ct
Copy link
Author

kmate-ct commented Feb 8, 2024

As far as I know maven_install doesn't use ijar anymore but their own simplified import rule, exactly because Scala macros and some issues with Kotlin as well. But yeah, it'd be great to make maven_install use scala_import and version the artifacts automatically, but based on the conversations I've seen I don't see much willingness to go to that direction.

@kmate-ct
Copy link
Author

Update: for now we went with maven_install, which means that I won't be able to do any more progress on this PR in the foreseeable future. (We missed some of the Scala specifics that bazel-deps has but managed to work around those for ourselves for now.)

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.

2 participants