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

go: use module sources digest for each of its packages #17728

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Dec 6, 2022

Problem

As reported in #17592, third-party Go modules are allowed to assume that all files packaged with the module will be visible during compilation. In that particular issue, the Confluent Kafka client library uses Cgo and expected a subdirectory with files used during C compilation will be visible to the package using Cgo.

Pants, however, subsets the modules sources digest to just the files in the exact subdirectory for each of the module's packages. This means that the subdirectory files were not visible when compiling the package in confluent-kafka-go in the immediate parent directory.

Solution

Since Pants cannot predict how a third party package will use the files in the modules sources, Pants cannot reasonably subset the modules source digest. Solve the issue by not subsetting the module sources digest to produce a package-specific digest.

This is a partial fix for #17592.

@tdyas tdyas added backend: Go Go backend-related issues category:bugfix Bug fixes for released features labels Dec 6, 2022
Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

I'm curious why we ever did this subsetting to begin with?

@tdyas
Copy link
Contributor Author

tdyas commented Dec 7, 2022

Because at the time I had the notion that package at a time compilation could be best served by only exposing the files in a package so Pants was not materializing the whole module's files for each package.

In hindsight this performance optimization compromised correctness for some modules.

@tdyas tdyas force-pushed the golang_pkg_can_see_whole_module branch 2 times, most recently from ce22d94 to c9ed349 Compare December 8, 2022 03:05
@tdyas tdyas force-pushed the golang_pkg_can_see_whole_module branch from c9ed349 to 09a75ef Compare December 8, 2022 22:59
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks! Given the perf cost of subsetting, this might end up being a wash for performance. Either way, you're right about correctness mattering more.

@tdyas tdyas merged commit 39ee73a into pantsbuild:main Dec 9, 2022
@tdyas tdyas deleted the golang_pkg_can_see_whole_module branch December 9, 2022 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Go Go backend-related issues category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants