Packages erroneously fail with multiple package main & build +ignore files

[kamilchm]

Hi,

I'm the author of https://github.com/kamilchm/go2nix which tries to generate Nix dependency manager derivations for Go projects. go2nix is a little hacky tool but it gets job done for almost all Go programs in Nix/NixOS https://github.com/NixOS/nixpkgs/search?utf8=%E2%9C%93&q=go2nix
I'm very happy with the new Go package management movement and I want to rewrite go2nix to use one common dependency solving code, which gps looks to be.
I tried to run gps example and it failed on my 2 first test cases:

1. Root project is github.com/cerana/cerana/cmd/bootserver

| | | | | | ✓ select github.com/spf13/afero@master w/1 pkgs
| | | | | | | ? attempt golang.org/x/text with 2 pkgs; 1 versions to try
| | | | | | | | try golang.org/x/text@master
| | | | | | | | ✗ golang.org/x/text at master has problem subpkg(s):
| | | | | | | | | 	golang.org/x/text/unicode/norm has err (*build.MultiplePackageError); required by github.com/spf13/afero@master.

2. Root project is github.com/miekg/coredns

| | | | | | | | | | | | | | | | | | | | | | | | | | | | | ? attempt golang.org/x/text with 2 pkgs; 1 versions to try
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | try golang.org/x/text@master
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | ✗ golang.org/x/text at master has problem subpkg(s):
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | 	golang.org/x/text/unicode/norm has err (*build.MultiplePackageError); required by github.com/PuerkitoBio/purell@v1.1.0.	golang.org/x/text/width has err (*build.MultiplePackageError); required by github.com/PuerkitoBio/purell@v1.1.0.
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | ← no more versions of golang.org/x/text to try; begin backtrack
panic: canary - *should* be impossible to have a pkg-only selection here

goroutine 1 [running]:
panic(0x7501c0, 0xc4201b69e0)
	/nix/store/kvs0532mn4ajscf43bkq2dbjry8cbb9f-go-1.7.4/share/go/src/runtime/panic.go:500 +0x1a1
github.com/kamilchm/gps2nix/vendor/github.com/sdboyer/gps.(*solver).backtrack(0xc420162900, 0xc420f97b40)
	/home/kamil/src/github.com/kamilchm/gps2nix/vendor/github.com/sdboyer/gps/solver.go:961 +0xaba
github.com/kamilchm/gps2nix/vendor/github.com/sdboyer/gps.(*solver).solve(0xc420162900, 0x0, 0x0, 0x80)
	/home/kamil/src/github.com/kamilchm/gps2nix/vendor/github.com/sdboyer/gps/solver.go:384 +0x7af
github.com/kamilchm/gps2nix/vendor/github.com/sdboyer/gps.(*solver).Solve(0xc420162900, 0x30, 0xc4200120dc, 0x18, 0xc420142cc0)
	/home/kamil/src/github.com/kamilchm/gps2nix/vendor/github.com/sdboyer/gps/solver.go:330 +0x8e
main.main()
	/home/kamil/src/github.com/kamilchm/gps2nix/main.go:44 +0x395

Is there something obvious in my examples that makes them fail?

I'm willing to help fixing gps and I think that good integration test based on a few real packages could help here. Is there something like that, which I can run and be sure I don't break anything?

[sdboyer]

Hi, welcome! Glad you're here!

Yes, I think gps would be well-suited for the case you're describing. In fact, it's exactly one of the kinds of integrations that I was hoping would arise.

The issue you're seeing is an unfortunate byproduct of gps still relying on build.ImportDir().

While the inline comments indicate that ListPackages() should be able to handle "one or more" of this case, but the bug you've raised here has demonstrated that that's a lie. The "one" case works fine, and has multiple test cases covering it...but the "or more" is the problem. golang.org/x/text/unicode/norm has two files which are package main and // +build ignore, so it's "or more."

I started writing a further hack to make this particular case work, but it just didn't quite seem worth it, being that my plan (#99) is to get rid of go/build entirely, and replace it with our own direct implementation using go/parser. However, that's a bigger undertaking, so I'd certainly welcome a PR in the meantime that just fixes the current logic to make the "or more" a true statement.

If you're interested in doing that, all it would take to make one of the existing test cases cover this particular problem is running this from the repo root:

$ cp _testdata/src/igmainlong/igmain.go _testdata/src/igmaint/igmain2.go

With that, go test -short should immediately begin failing with the same *build.MultiplePackageError) err. I've no problem with extending that test case to cover this; alternatively, you could make a wholly new subdir under _testdata/src, with the right combination of files. That would entail require setting up another fixture declaration in TestListPackages(), though.

[sdboyer]

And, more generally - yes, I absolutely welcome reports of gps dealing with real code improperly! I expect there's going to be quite a few more such reports once we open up the new tool (very soon), but that's no reason not to get them in now. Compiling and running the example.go can be a great way of sussing out a subset of the possible issues, as this issue demonstrates.

wrt basing tests on real packages - while I'm all for running the code against them to discover problems, in general I prefer to base the tests on mocked data where possible. This keeps testing times down, and maximizes failure locality.

[kamilchm]

What do you think about respecting build tags strictly in build.Context by removing UseAllFiles from

gps/analysis.go

Line 74 in 4af1eef

ctx.UseAllFiles = true

?

[sdboyer]

@kamilchm that won't help either, unfortunately. It would result in solutions that are arch-specific, which is expressly not the goal. go/build just isn't designed for this use case, and why we ultimately have to write our own from go/parser.

[kamilchm]

It works for my own one platform after that. I know it could smells bad but how about setting all archs in build.Context.BuildTags instead of UseAllFiles?
Do you see any other way to make it work beside the rewrite to own go/parser?

[sdboyer]

Right, that's the other part - if we turn off UseAllFiles, then it's not just os/arch that we miss - it's also any other build tag that's not explicitly mentioned. That's an unrestricted set, so we can't enumerate them.

This approach fits the purpose of build.Import() well: assemble a build.Package that works given the current inputs. gps' goal, however, is to create a picture of all the import statements that are present in a directory, parameterized by the context (build, os/arch, and release tags) of their inclusion (#44).

This is the goal because doing otherwise opens up the possibility of generating an incomplete (i.e. imports unique to a given build tag are missed, but the user actually needs that build tag in their chosen workflow) or non-portable Solution. Doing so would introduce both new user-facing failure modes and additional user choice that, in the cases I've seen thus far, does not appear to actually add enough value to outweigh the complexity cost of foisting more choice on the user.

Because these use cases vary so significantly, I view ListPackages() as being a strongly-opinionated sibling to build.Import() that establishes its own semantics for this problem space. Thus, I'm pretty set on refactoring to work directly atop go/parser. That said, some counterarguments I'd find effective:

• The possible combinatorial explosion of build tags is concerning, and I've not worked out the math to determine how bad it might get. A formal-ish description of it getting really bad, really fast in non-intentionally crazy situations could dissuade me from this whole approach.
• An empirical demonstration that the Go ecosystem, as it exists today, is likely to have lots and lots of diamond dep conflicts if we always try to solve across all os/arch/build/release combos. I realize this would require considerable effort to demonstrate...but that's why it would be valuable, and also why I haven't had a chance to do it myself yet! (I haven't looked at BigQuery, I wonder if it could be shoehorned to this purpose...)
• I could be convinced to rearchitect solving in a way that DOES explicitly produce non-portable solutions. It'd be a significant refactor, though. This would be a lower priority for me personally, because I don't think we have the case for it in a dev-facing tool, but I can easily imagine how something like Nix might have different needs. This refactor would entail:
  • Keep tags on selected deps so we know the context of their inclusion #44 would probably be a blocker, because we currently don't track this information at all anyway
  • SolveParameters takes some tag limiting options (probably analogous to build.Context.BuildTags and build.Context.ReleaseTags)
  • ListPackages() is expanded to take a similar list of limiting parameters
  • Solution, and possibly even Lock, would need to be expanded to declare the scope in which it applies, as defined by these parameters. (We could say that right now the scope is implicitly "universal").

All that's pretty hard, though :) In the meantime, the existing logic could be modified to encapsulate the ignored-main-finding logic in a closure, then looping and re-running build.ImportDir() with one more file eliminated from consideration each time. There's a little more fiddling to do to manage all the vars correctly, but it should work. (That's the hack I started implementing last night).

[kamilchm]

imports unique to a given build tag are missed, but the user actually needs that build tag in their chosen workflow

Given our example how would I tell gps that I don't want ignored dependencies?
How would that user tells what builg tags are needed in his workflow? Is there any other way to satisfy this use case without exposing build tags to user?
It's also inefficient to traverse all files when user knows what tags are his actual needs.
Do I understand that use case incorrectly?

[sdboyer]

Given our example how would I tell gps that I don't want ignored dependencies?

You can't, but that's not relevant in the particular example you gave. The logic just needs to change to handle more than one file following this pattern - as the comments indicate it should.

How would that user tells what builg tags are needed in his workflow?

I detailed a possible approach to this in my last comment, but again, it's not relevant to solving the specific problem you encountered.

Is there any other way to satisfy this use case without exposing build tags to user?

Yes, again, make the existing logic work when there's more than one file with package main and +build ignore in a given dir.

It's also inefficient to traverse all files when user knows what tags are his actual needs.

Safety and completeness are the goal here; efficiency is relevant once they've been satisfied. (go/build is also potentially quite inefficient compared to what we could write with our own implementation).

Do I understand that use case incorrectly?

I'd say that's possible, judging from these questions.

[kamilchm]

Is there a doc or test that show the use case for ignoring packages like described here https://github.com/sdboyer/gps/wiki/gps-for-Implementors#ignoring-packages?

I looked into test cases and see that gps would treat imports from ignored packages as any other dependency

gps/analysis_test.go

Line 608 in 65939b4

"code and ignored main": {

If I understand it correctly, even when we do looping and re-running build.ImportDir() it will read imports from these ignored files?

[sdboyer]

I looked into test cases and see that gps would treat imports from ignored packages as any other dependency

Not quite. ListPackages() does not consider ignores, because it is a lower-level system - it is operating on files (a "physical" construct), whereas ignores operate on import paths (a logical overlay on files). And the test case you referenced is a part of TestListPackages().

Ignores come into play when we calculate the set of import paths we actually care about from the PackageTree produced by ListPackages(). That's done by PackageTree.ExternalReach(), which takes an ignore map as its third parameter. The results are as described in the documentation. Ignores are also tested as part of the full solve-level integration testing suite.

If I understand it correctly, even when we do looping and re-running build.ImportDir() it will read imports from these ignored files?

Yup. Because the goal of ListPackages() is to completely and accurately characterize what is present on disk. In the eventual go/parser-based implementation, we would not "drop" these +build ignore files at all, but record the fact of their existence (and their imports) in such a way that the solving algorithm can later decide to drop the information.

For now, though, we munge the import paths together from the main and non-main files, with the idea that it's less harmful to (potentially) have some extra unused deps than it is to make the main files non-buildable with the Solution we produce.

[sdboyer]

@rsc this discussion, particularly this comment, might be useful in your thinking about this problem space.

[kamilchm]

@sdboyer thanks for explanation.
I'll try to dive deeper and check how can I use gps 👍

[sdboyer]

This ended up being fixed by #149 :)