DO NOT MERGE: Fix issue where transitive dependencies in a workspace were not added to ModuleFileSet #2529
Conversation
… to ModuleFileSet
bufdev
changed the title
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.
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.
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.
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.
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.