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

merge ianoc with master #321

Merged
merged 28 commits into from
Feb 28, 2023
Merged

merge ianoc with master #321

merged 28 commits into from
Feb 28, 2023

Conversation

johnynek
Copy link
Collaborator

I did a minimal amount of change to get the code to pass tests (and rewrote some custom implementations of traverse).

@johnynek johnynek mentioned this pull request Feb 28, 2023
@johnynek
Copy link
Collaborator Author

I think the main thing to review is here:

https://github.com/bazeltools/bazel-deps/pull/321/files#diff-39276c5898c0acd1208cf3c9b195c48fe3739d50a8294849bf5bdcc2e2bc390c

There are a lot of formatting changes you can ignore.

Comment on lines +60 to +76
// Gradle has compile/runtime/test sepearate classpaths
// we just want one, so we merge them all
private def collapseDepTypes(
lockFile: GradleLockFile
): Try[Map[String, GradleLockDependency]] =
List(
lockFile.compileClasspath,
lockFile.annotationProcessor,
lockFile.resolutionRules,
lockFile.runtimeClasspath,
lockFile.testCompileClasspath,
lockFile.testRuntimeClasspath
)
.map(_.getOrElse(Map()))
.foldLeft(Try(Map[String, GradleLockDependency]())) { case (p, n) =>
p.flatMap { s => TryMerge.tryMerge(None, s, n) }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the transitive dependencies of test dependencies could override the compile configuration? This could potentially cause prod differences between Gradle and Bazel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that's what it means. It's intentional. We can discuss later. We want a single version of all the jars. In fact, gradle having a different version for each project main and test has caused prod issues: we know these jars will be combined at runtime, so ignoring that and letting them transitively get different dependencies at compile and test time is asking for trouble.

Copy link
Contributor

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Left a comment about configuration merge.

@johnynek johnynek merged commit e589b9a into master Feb 28, 2023
@eed3si9n eed3si9n deleted the oscar/20230227_merge_ian_master branch February 28, 2023 18:56
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.

5 participants