-
Notifications
You must be signed in to change notification settings - Fork 244
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
DO NOT MERGE: Fix issue where transitive dependencies in a workspace were not added to ModuleFileSet #2529
Conversation
… to ModuleFileSet
private/buf/bufcli/bufcli.go
Outdated
@@ -404,7 +404,7 @@ func NewWireImageConfigReader( | |||
storageosProvider, | |||
NewFetchReader(logger, storageosProvider, runner, moduleResolver, moduleReader), | |||
bufmodulebuild.NewModuleBucketBuilder(), | |||
bufimagebuild.NewBuilder(logger, moduleReader), | |||
bufimagebuild.NewBuilder(logger, moduleReader, bufmoduleref.NewModulePinResolver(clientConfig)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you match the style in the codebase and either have all args on one line, or all args separated by newlines?
@@ -327,6 +329,46 @@ func NewProtoModulePinsForModulePins(modulePins ...ModulePin) []*modulev1alpha1. | |||
return protoModulePins | |||
} | |||
|
|||
// ModulePinResolver is a resolver for module pins. | |||
type ModulePinResolver interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be adding this to bufmoduleref
- if anywhere, it would go in bufmodule
, and probably just be another method on ModuleResolver
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't even know that existed, TIL. I'll do this up.
Still one more test case I'm missing (identities match but no pin conflict), but take another look. I am uneasy about how it's tested, I'll look later and see if there's a cleaner way to test that, perhaps at a higher layer. |
I see you merged in main and now |
Figured it out; in the past we weren't properly adding workspace deps, but now we are. The "pre-formatted" image has a workspace (because it's in the repo), whereas the "formatted" image does not have a workspace (because it's in a temp dir outside the repository). So naturally, the images are different. I'll fix the test, but all the testdata implicitly having a workspace is bad... |
@doriable @oliversun09 @jchadwick-buf requested review from you all, @bufdev is busy for the rest of the week. Take a look whenever you get a chance, please :) Don't worry about the Windows test, I'll fix it up, it's some strange path issue. |
// through dependency resolution. | ||
var ( | ||
dependencyModulePins []bufmoduleref.ModulePin | ||
seenDependencyModulePinIdentities = make(map[string]bufmoduleref.ModulePin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: perhaps something like seenDependencyIdentityStringToPin
to indicate it's a map?
@oliversun9 any idea why the windows test is failing? I've spent some time looking and I can't seem to track it down. Not being able to run it locally makes it challenging. |
I notice that in these tests, a workspace is always built -- this is because of the buf.work.yaml at the root of bufbuild/buf repository. This is so far the only way I can think of in which this PR can affect the failing test. I will keep looking, but just post this update in case it rings a bell. |
Declining this PR in favour of bufmod, which re-architects how modules are constructed and images are built. |
Fixes #2513.
Issue: does not deal with transitive deps with different commits for same module.