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

Ignore OS as an input to GenerateProtoTask #2

Closed
wants to merge 1 commit into from

Conversation

clayburn
Copy link
Owner

This change will remove the OS from the input properties of the
GenerateProtoTask when protoc is defined as a resolvable artifact. The
expectation is that same versions of protoc will produce same outputs
regardless of the OS on which it was run.

This is done by removing the "protoc" key from the alternativePaths
property when present and not a SNAPSHOT and adding a protocArtifactGav
property as an input that only considers the group, artifact ID, and
version of the resolvable protoc.

See google#457.

This change will remove the OS from the input properties of the
GenerateProtoTask when `protoc` is defined as a resolvable artifact. The
expectation is that same versions of protoc will produce same outputs
regardless of the OS on which it was run.

This is done by removing the "protoc" key from the `alternativePaths`
property when present and not a SNAPSHOT and adding a protocArtifactGav
property as an input that only considers the group, artifact ID, and
version of the resolvable `protoc`.

See google#457.
Copy link

@runningcode runningcode 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

return objectFactory.fileCollection().from(getLocatorToAlternativePathsMapping().get().values())
objectFactory.fileCollection().from(locatorToAlternativePathsMapping.map {
if (protocArtifactGav.map { !it.endsWith("-SNAPSHOT") }.getOrElse(false)) {
it.findAll { it.key != protocLocator.get().name }

Choose a reason for hiding this comment

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

Could you add a comment explaining what the logic is and why it is that way? I think this would help future readers or when trying to debug things here.

It would also be good here to name parameters since I think we have nested it and it becomes hard to follow which one is which.

@clayburn clayburn closed this May 3, 2022
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