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

cmd/go: outer module can provide packages that appear to be in an inner module #29736

Open
heschi opened this issue Jan 14, 2019 · 7 comments
Open
Labels
Documentation GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@heschi
Copy link
Contributor

heschi commented Jan 14, 2019

Consider two modules, one of which is nested in the other:

modcache/pkg/mod/foo.com
├── outer
│   └── inner@v1.0.0
│       ├── go.mod
│       └── p2
│           └── y.go
└── outer@v1.0.0
    ├── go.mod
    └── inner
        └── p
            └── x.go

Module foo.com/outer/inner contains p2, which would be imported as foo.com/outer/inner/p2. Module foo.com/outer contains inner/p, which would be imported as foo.com/outer/inner/p.

I think most people would find this surprising. I certainly did. I had thought that the presence of a nested module "punched a hole" in the containing module, but I guess that's only true if the go.mod file is actually present in the source tree of the containing module? So if the two modules are developed on separate branches, as would have happened here, the hole-punching behavior doesn't happen?

Is this a bug, or expected behavior?

@bcmills, @jayconrod

@bcmills
Copy link
Contributor

bcmills commented Jan 14, 2019

This is working as designed. Consider what happens if you have a stable outer module and an unstable inner module, and want to promote outer/inner/p to be stable.

One way to do that would be to create three modules:

  • outer@v1.X.0
  • outer/inner@v0.Y.0
  • outer/inner/p@v1.0.0

As you move packages back and forth, you end up creating a module for every directory — not a great experience for the maintainer.

In contrast, with this sort of interleaving, you can have:

  • outer@v1.X.0
  • outer/inner@v0.Y.0

and just move outer/inner/p from the v0 module into the v1 one.

I expect that would be particularly useful when inner is, for example, an internal package factored out of outer so that outer and inner can share implementation.

@hyangah
Copy link
Contributor

hyangah commented Jan 14, 2019

This is worth mentioning in the Module wiki or the future Module best practice wiki.

@bcmills
Copy link
Contributor

bcmills commented Jan 14, 2019

CC @thepudds @myitcv

@jayconrod
Copy link
Contributor

I think this behavior is too confusing. I'd be much happier in a world where each module in the build list is a namespace of packages. You would be able to find out what module a package belongs to just by finding the longest matching prefix.

This also has an I/O cost. In order to determine whether a module contains a package, we need to list the directory, and then parse and apply build tags to confirm the package is buildable. I'm hoping that cost will be reduced with additional caching in 1.13, but still.

@bcmills
Copy link
Contributor

bcmills commented Jan 14, 2019

“longest matching prefix” seems like a non-starter. That would make it impossible to recombine modules that have been split. (Recombination is important: without it, any time you make a change to the internal interactions between two packages, you have to remember to tag and update the requirements of all of the involved modules.)

The I/O cost should not be terribly high. I don't think we should need to parse or apply build tags: it should be fine to issue an ambiguous import error if there are Go source files at the same import path in two different modules, even if one of the variants happens to be unbuildable in the current configuration.

@jayconrod
Copy link
Contributor

Hmm, recombination is really important.

Let's suppose we had simpler "longest prefix" module resolution, and we didn't issue an error for ambiguous imports. If I import foo.com/outer/inner/p, that would definitely get resolved to foo.com/outer/inner as long as I have that in my go.mod. To recombine these modules, the maintainer of foo.com/outer would need to copy everything from foo.com/outer/inner, issue a new version, then tell users to stop requiring foo.com/outer/inner. The drawback of this approach would be that incremental migration is difficult for users.

The incremental migration approach described above is pretty difficult, too, though. In order to promote a package from module foo.com/outer/inner to foo.com/outer, the maintainer needs to issue new tags for both, and the user needs to update both modules to avoid ambiguous import errors. It seems like a breaking change to remove a package from foo.com/outer/inner, but if both modules depend on each other at newer versions, I don't think anything would actually break.

Is there any better way?

In any case, +1 to Hana's suggestion document things like "how to recombine modules" in a "best practices" document.

@bcmills
Copy link
Contributor

bcmills commented Jan 14, 2019

In order to promote a package from module foo.com/outer/inner to foo.com/outer, the maintainer needs to issue new tags for both,

Yep! (But, importantly, they only need to do that once, not every time they change the package thereafter.)

and the user needs to update both modules to avoid ambiguous import errors.

Also true. See also #27899.

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 28, 2019
@seankhliao seankhliao added this to the Unreleased milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation GoCommand cmd/go modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants