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: add a flag to ignore build constraints when listing packages #42504

Open
mvdan opened this issue Nov 11, 2020 · 30 comments
Open

cmd/go: add a flag to ignore build constraints when listing packages #42504

mvdan opened this issue Nov 11, 2020 · 30 comments
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Nov 11, 2020

I realise that the point of go list is to list packages, and I realise that one must generally obey build constraints when loading packages. For example, it makes no sense to try to type-check all the Go files in a package while ignoring build constraints, because there will likely be duplicate definition errors if declarations are split by GOOS or GOARCH.

Having said that, it can be very useful to merely list or traverse a set of packages in a way that build constraints are completely ignored. The most common use case is: what packages are potentially imported by the current package, directly or indirectly, across all build configurations?

This question is valid, for example, if one wants to copy a package and all of its transitive package dependencies. go list -deps <package> does not work in general, because if I run the command on Linux, I would not be including imports which are only used on Windows.

The closest thing we have right now is go mod vendor, which copies all transitively imported packages by all the packages in the current module into the vendor folder, across all build constraints. So cmd/go already has the machinery to traverse a package dependency graph while ignoring build constraints, it seems.

--

Here ends the problem statement, and begins my initial suggestion for a solution: a go list flag to ignore build constraints. Here are some example use cases:

  • List all the packages potentially depended on by a given package: go list -deps -newflag <package>

  • List all the packages under the current directory tree, not just for the current platform: go list -newflag ./...

I'm not sure what this new flag could be called. Perhaps -nobuild or -anybuild.

The flag would also restrict what go list can do. For example, using -newflag along with -export or -compiled would be an error, because it does not make sense to load/build packages when we're ignoring build constraints. Similarly, -newflag -json would always omit some fields like IgnoredGoFiles, since they make sense only when following build constraints.

This idea has been discussed briefly before, but as far as I know no problem statement or solution has been raised as an issue yet. I'm not using a proposal title and label; as far as I know that's only necessary if we need the proposal review committee to intervene.

cc @bcmills @jayconrod @matloob @rsc for cmd/go
cc @ianthehat @dominikh @myitcv @rogpeppe @kardianos for golang-tools and some previous issues

@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. GoCommand cmd/go Tools This label describes issues relating to any tools in the x/tools repository. labels Nov 11, 2020
@mvdan
Copy link
Member Author

mvdan commented Nov 11, 2020

The main counter-argument I see to this flag solution is that it could make go list harder to understand. Right now, it always obeys build constraints when looking at packages, and it only has a second mode: -m for instead working with modules.

However, I think this third mode would be fairly niche, and we could explain it rather simply: list packages while ignoring build constraints, hence not loading packages as if they were to be built. This helps us limit go list -newflag to only support basic operations.

@rogpeppe
Copy link
Contributor

I support the idea here. I've often wanted to enumerate dependencies across all build configurations and it's awkward to do currently (I've built custom solutions to do it in the past).

As for the name of the flag, how about -all-builds ?

@mvdan
Copy link
Member Author

mvdan commented Nov 11, 2020

The only reason I tried to avoid "all builds" as a flag name is because, to me, that implies "load and possibly build all build constraint combinations". As if go list -export -all-builds would then build an export file for each GOOS/GOARCH platform, for example.

@kardianos
Copy link
Contributor

For govendor, years ago, I had to re-build go list functionality because of this very issue, but that was specific for vendoring that as you note go mod handles correctly. Initially go code completion tools also (unfortunately) followed build constraints, but that seems to have been fixed now in recent gopls. So I agree there is strong data points support this need in general. I cannot comment on the addition to go list itself. But I could easily believe this would be useful to have.

@mvdan
Copy link
Member Author

mvdan commented Nov 11, 2020

I hadn't thought about gopls. Perhaps they have some code that could be eventually replaced with this new flag. cc @stamblerre

@jayconrod
Copy link
Contributor

This would be useful. We already have some of the code for this in cmd/go for module commands.

In go mod tidy, the go command loads packages in all and their tests, ignoring build constraints with one exception: the tag ignore is still considered false. That's useful for examples and small one-file programs that are in the same directory as another package. If there's a flag to ignore build constraints, I think it should have the same semantics.

As for the CLI, maybe overload -tags? Something like -tags=all or -tags=*?

This should be restricted to go list, too, and I agree about restricting -export and -compiled.

@mvdan
Copy link
Member Author

mvdan commented Nov 11, 2020

the tag ignore is still considered false. [...] If there's a flag to ignore build constraints, I think it should have the same semantics.

That would be fine for my use case. The packages I want to list will eventually be loaded in some build configuration, so the ignore tag will never really play a part in that. I guess that, if someone has a very niche use case where they want all files including those with +build ignore, they could use go list -f {{.Dir}} -newflag and then grab/copy the entire directory.

As for the CLI, maybe overload -tags?

Overloading -tags could be a nice idea. I only worry that it's a generic build flag, so what would go build -tags=all do? Do we define that go list's -tags flag is actually different than all the other occurrences of the same flag?

@mvdan
Copy link
Member Author

mvdan commented Nov 11, 2020

they could use go list -f {{.Dir}} -newflag and then grab/copy the entire directory.

To clarify in case anyone wonders - this kind of workaround about copying entire directories doesn't help solve the issue in this thread. For example, think packages which contain zero files matching the current build constraints, which wouldn't show up in the go list output at all.

@bcmills
Copy link
Contributor

bcmills commented Nov 11, 2020

We will need a bit of care with import cycles, though. When ignoring build tags, it is possible to end up with import cycles that are otherwise rejected.
(See https://github.com/golang/go/blob/master/src/cmd/go/testdata/script/mod_tagged_import_cycle.txt.)

@ianthehat
Copy link

It transforms the ouptut of go list from a DAG to a true graph, and there are real world cases of this. It would never be feasible to have a version of the go/packages API that sat on top of this for instance (which if it was just a special tag would end up happening)
I do think that it would be a valuable feature, but it really does need to be called out as a special case that you have to hold carefully, because its the kind of thing that is easy to use in a way that works on your testing and dies in the real world in hard to comprehend ways.

@bcmills
Copy link
Contributor

bcmills commented Nov 11, 2020

@ianthehat, FWIW I suspect that the existing -e flag also transforms the output from a DAG to a graph, or at least would if it worked as often as we want it to. (Cycles cause an error, but -e suppresses errors.) So tools built on top of go list -e probably already need to know how to handle cycles, and I'm not sure how many tools build on go list today don't want -e.

@stamblerre
Copy link
Contributor

stamblerre commented Nov 12, 2020

To answer #42504 (comment), gopls actually doesn't have any handling for build tags (hence the longstanding #29202). Users must specify their build tags in their configuration before using build tagged files. If it were possible to use this in go/packages, gopls might be able to use it when we do get around to #29202 -- but there are a lot of other complications with type-checking that we would need to figure out first.

@mvdan
Copy link
Member Author

mvdan commented Jan 14, 2021

We briefly spoke about this on yesterday's tools call, and we seemed to agree that there is consensus that we should do this. We've found at least two good use cases: enumerating dependencies across all build environments, and support for build-tag-agnostic features in go/packages or gopls.

Tentatively moving this to NeedsFix and milestoning for 1.17.

@mvdan mvdan added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 14, 2021
@mvdan mvdan added this to the Go1.17 milestone Jan 14, 2021
@rsc
Copy link
Contributor

rsc commented Jan 19, 2021

Assuming this is exactly what tidy and vendor do (not quite "ignore all constraints" because we would still respect // +build ignore), this seems worth doing.

What to call the flag? Internally we make this work by setting a special tag *, which we ignore on the command line now. But we could make it do the internal thing, so how about -tags '*'? People who see that will have a decent idea what it means.

@mvdan
Copy link
Member Author

mvdan commented Jan 19, 2021

I slightly worry that some users might confuse that with -tags *, which will do something entirely different in most shells. I don't have a better suggestion, though.

@rsc
Copy link
Contributor

rsc commented Jan 19, 2021

@mvdan, at least -tags * will not break silently :-)
I'm OK with something else like -tags any though.

@mvdan
Copy link
Member Author

mvdan commented Jan 23, 2021

I lean towards -tags any. I hope noone is using // +build any, as that would be pretty confusing in itself.

@ianlancetaylor
Copy link
Contributor

Is this going to happen for 1.17?

@mvdan
Copy link
Member Author

mvdan commented May 4, 2021

I don't believe anyone has started work on this, and as a new feature it seems like it should wait until 1.18.

@bcmills bcmills modified the milestones: Go1.17, Backlog May 4, 2021
@gopherbot
Copy link

Change https://golang.org/cl/332571 mentions this issue: cmd/go/internal/modload: remove ImportMap and PackageDir

@yarikk
Copy link

yarikk commented Jul 7, 2021

Any update on this? While we wait on this, we've planted a very slow workaround: repeat go list -tags for every platform combination reported by go tool dist list. Can't wait to replace with proper go list -tags any.

@bcmills
Copy link
Contributor

bcmills commented Nov 10, 2021

This unfortunately didn't make 1.18.

@yarikk, note that another workaround is to run go mod vendor and then scrape the list of packages from vendor/modules.txt. (It's admittedly ugly, but it should work...)

@sify21
Copy link

sify21 commented Nov 11, 2021

This unfortunately didn't make 1.18.

@yarikk, note that another workaround is to run go mod vendor and then scrape the list of packages from vendor/modules.txt. (It's admittedly ugly, but it should work...)

@bcmills, does go list -m and go mod graph ignore build constraints when working with modules?
Also I can't find the explanation of modules.txt file format, is it repeated blocks of the following?

# module_name module_version
## explicit
pakcage_name
...

@findleyr
Copy link
Contributor

findleyr commented Jan 19, 2022

By providing a mechanism to list all files, gopls could theoretically implement package construction in a post-processing pass. This could facilitate several gopls features:

  • Improved build tag support (x/tools/gopls: improve handling for build tags #29202). We could dynamically reorganize packages as build tag configuration changes (or based on the set of open files).
  • Improved support for //go:build ignore single-file packages
  • Using gopls to target older go versions. Right now we can set the language version (types.Config.GoVersion), and warn about standard library API use, but have no mechanism to simulate the set of files that would be loaded with a different set of release tags (consider that a file could be //go:build go1.15 && !go1.18, so there is currently no way for us to see this file at 1.18).

@liggitt
Copy link
Contributor

liggitt commented Apr 13, 2022

if go list is positioned as the preferred approach to determine canonical dependencies of a module (as in #47648), the ability to determine this across all possible build tags is necessary

since go1.17, go mod graph hasn't been useful for determining direct dependencies of a module, and I don't see a 1:1 replacement in go list as long as this issue is unresolved

@yarikk
Copy link

yarikk commented May 11, 2022

This unfortunately didn't make 1.18.

@yarikk, note that another workaround is to run go mod vendor and then scrape the list of packages from vendor/modules.txt. (It's admittedly ugly, but it should work...)

As we're going to run this on a big tree of vendored packages, I anticipate this is going to be much slower than our current workaround, which is calling go list multiple times, each time with different set of platform -tags, for all the platforms we care about, and then merging the results appropriately.

Would love to see the proper solution, as @rsc had suggested above

@bcmills bcmills modified the milestones: Go1.19, Go1.20 May 25, 2022
gopherbot pushed a commit that referenced this issue Aug 23, 2022
These two functions together duplicated much of the functionality of
modload.Lookup. Use that instead in modcmd.vendorPkg, and reduce the
modload surface area.

Updates #42504
Updates #40775
For #26904

Change-Id: Ib8aaac495d090178dd56971aef9e5aa44ffa818b
Reviewed-on: https://go-review.googlesource.com/c/go/+/332571
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
@bcmills bcmills modified the milestones: Go1.20, Go1.21 Dec 12, 2022
@yarikk
Copy link

yarikk commented Feb 2, 2023

Sorry to see this pushed back again, given that the functionality is already there, from what had Russ mentioned above.

We expect the result of this implemented should considerably improve developer's experience for monorepo cases, reducing scanning times of (big amounts of) vendored code.

@mvdan
Copy link
Member Author

mvdan commented Feb 2, 2023

@yarikk this feature would indeed be very helpful. If you want it to be implemented faster, giving a thumbs up on the original post or volunteering as a code contributor are the two options that come to mind. Beyond that, I don't think that commenting regularly is going to help :)

@adg
Copy link
Contributor

adg commented May 24, 2023

I started investigating this, and have a dirty hack that makes -tags any work.

My first change was to overload the -tags flag so that the value any has the special side effect of setting the cfg.BuildContext.UseAllFiles field to true, and doesn't update cfg.BuildContext.BuildTags. This means that all users of that build context to enumerate packages or files always see everything.

The next change is to make loadTags in internal/imports return AnyTags() if cfg.BuildContext.UseAllFiles is true.

And the final change is to make the eval method in internal/modindex behave the same way as eval in internal/imports, by treating the * tag as meaning "anything except ignore evaluates to true".

Problems here:

  • Setting cfg.BuildContext.UseAllFiles to true has the unfortunate side effect that all users of that context will see files with an ignore build tag, which is probably not desirable in all situations: there are over 100 references to cfg.BuildContext in cmd/go/internal, I have only looked at a few of them in any detail.
  • The major complication with all of this is the conflict between go/build and its various partial modified copies or reimplementations in cmd/go/internal.
    • It would really help to consolidate the duplicated stuff inside cmd/go/internal - that there are two versions of the build constraint evaluator, one in imports and one in modindex is a bit of a hindrance.
    • If go/build would treat -tags=* in the same way as the internals of cmd/go, then I think a lot of this would become easier.

@bcmills
Copy link
Contributor

bcmills commented Mar 14, 2024

@matloob, @samthanawalla: if I recall correctly, this issue is mostly just waiting on a decision about how to specify the flag, and this would be a nice usability improvement for tools & scripts that use go list to enumerate packages.

(Otherwise, those scripts have to try various package-specific combinations of build tags to get the complete set that will be considered by go mod tidy.)

This may also be needed for gopls to properly self-configure for modules (CC @findleyr @adonovan).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests