x/pkgsite: support different GOOS/GOARCH values when displaying package documentation

[ChrisHines]

What did you do?

• Try to view operating system specific package documentation on the pkg.go.dev discovery site.
• For example os/exec.LookPath has different docs on Linux and Windows. Compare
  • https://golang.org/pkg/os/exec/?GOOS=linux#LookPath
  • https://golang.org/pkg/os/exec/?GOOS=windows#LookPath

What did you expect to see?

An easy to discover and use way to see OS specific package documentation on the pkg.go.dev discovery site.

What did you see instead?

No documented or otherwise discoverable solution on the pkg.go.dev discovery site.

Notes

This idea extends to GOARCH specific or build tag specific docs.

[dmitshur]

Thanks for filing this feature request! I agree, this would be nice to have.

/cc @julieqiu @dmitshur

[dolmen]

Here is a package where documentation rendering is GOARCH dependent: https://pkg.go.dev/github.com/dolmen-go/endian/?tab=doc

[jba]

This is trickier to implement than it might seem, because we want to do as much preprocessing as possible on the worker. If we just generated doc from source at serving time, this would be easy, but that's too slow. We would like to at least parse the files on the worker, and generate doc from the ASTs on the frontend.

A .go file with the contents

// +build ignore

Not valid Go code.

is perfectly fine and won't interfere with compiling, unless the ignore build tag is explicitly provided. That means that we can't just parse every file in a package; some might not be valid Go, and that's legal.

That means that we need a list of valid GOOS/GOARCH combinations. We could parse a file if it matches at least one combination. That list can be large, but not unbounded, so you won't be able to get docs for an arbitrary combination.

You could still have two disjoint sets of files, each with a different package name. So what do we record as the name of the package? You could argue that that's not a reasonable setup, and we could just take the first name we find.

[bcmills]

That means that we need a list of valid GOOS/GOARCH combinations.

Note that you can obtain such a list by running go tool dist list.

[myitcv]

That means that we need a list of valid GOOS/GOARCH combinations

This is related to a proposal that @mvdan and @dominikh put forward (but since retracted I think)

[mvdan]

Yep. We bounced a few ideas on what the next iteration could be; mainly, just focusing on go list and not trying to be compatible with other build systems. I can bring up a draft on the next tools call if that sounds interesting.

Edit: the retracted proposal was https://github.com/golang/go/issues/39005.

[gopherbot]

Change https://golang.org/cl/258737 mentions this issue: internal/godoc: remember and check GOOS/GOARCH

[firelizzard18]

A decent intermediate/minimum viable change would be a predefined set of tag sets. GOOS=linux|windows|darwin and GOARCH=amd64 would cover just about everything I deal with, except TinyGo.

On a related note, it would be nice if there was a way to indicate what the default GOOS should be. For example, https://pkg.go.dev/github.com/keybase/go-keychain is pretty useless for anything other than darwin.

[gopherbot]

Change https://golang.org/cl/279457 mentions this issue: internal/fetch: report DocTooLarge error in goPackage.err

[gopherbot]

Change https://golang.org/cl/279458 mentions this issue: internal/fetch: loadPackages returns multiple packages

[gopherbot]

Change https://golang.org/cl/288216 mentions this issue: internal/postgres: make query work with multiple build contexts

[gopherbot]

Change https://golang.org/cl/288217 mentions this issue: many: allow for multiple documentations for a Unit

[gopherbot]

Change https://golang.org/cl/288831 mentions this issue: internal/postgres: handle multiple documentations for a package

[gopherbot]

Change https://golang.org/cl/289238 mentions this issue: internal/frontend: support GOOS/GOARCH query params

[gopherbot]

Change https://golang.org/cl/289676 mentions this issue: internal/fetch: don't save build context in doc source

[gopherbot]

Change https://golang.org/cl/289672 mentions this issue: internal/fetch: clarify loadPackage error handling

[gopherbot]

Change https://golang.org/cl/289675 mentions this issue: internal/fetch: track build context file sets

[gopherbot]

Change https://golang.org/cl/289674 mentions this issue: internal/fetch: refactor file matching for loading a package

[gopherbot]

Change https://golang.org/cl/289673 mentions this issue: internal/fetch: refactor package loading

[gopherbot]

Change https://golang.org/cl/289678 mentions this issue: internal/godoc: remove GOOS and GOARCH from source

[gopherbot]

Change https://golang.org/cl/289680 mentions this issue: internal/fetch: collapse identical build contexts

[gopherbot]

Change https://golang.org/cl/289989 mentions this issue: internal/postgres: remove rows from documentation before inserting

[gopherbot]

Change https://golang.org/cl/290094 mentions this issue: internal: support "all" in BuildContext

[gopherbot]

Change https://golang.org/cl/290095 mentions this issue: internal/frontend: show build context on doc page

[jba]

UI design: top right corner of Documentation section will show:

• nothing, if the only build context is all/all (the current behavior)
• static text like "GOOS=js, GOARCH=wasm" if there is otherwise only one build context (and remove the similar text now at the bottom)
• a dropdown labeled "GOOS/GOARCH" with the available pairs ("linux/amd64", etc.)

[tianon]

• a dropdown labeled "GOOS/GOARCH" with the available pairs ("linux/amd64", etc.)

Just to clarify, did you really mean this dropdown will include all 41 platform combinations listed by go tool dist list, or is the intention to use some contextual clues (file names, build expressions, etc) to filter that list to a more user-friendly subset that might actually change the documentation?

[firelizzard18]

This seems to not be working as expected: https://pkg.go.dev/github.com/kardianos/service?GOOS=windows

github.com/kardianos/service successfully compiles with GOOS=windows. It has platform-agnostic files, and Windows-specific files.

[jba]

@tianon We currently only check a small subset of that list.

Currently we recognize the three cases I mentioned. In particular, if the docs differ for any of our checked build contexts, then we'll record them all. That's not ideal but with our short list it's manageable.

[jba]

@firelizzard18 This feature isn't officially live yet. We need to reprocess all packages to make it work. That will happen soon.

[gonutz]

I know that this is still an active thing, I just hit the same problem and would like to reinforce what looks like the solution you are already thinking about.

I just added module support for my Windows GUI library wui and in my readme linked to https://pkg.go.dev/github.com/gonutz/wui/v2 which does not show much because almost all my files are tagged //+build windows. After thinking long about what the problem might be I remembered there to be this URL ? option thing in godoc.org so I tried it here and it worked. Now I link to https://pkg.go.dev/github.com/gonutz/wui/v2?GOOS=windows and it works. This is however not easily set in the UI. A drop-down menu for all the existing combinations of GOOS and GOARCH in a repo would be nice (not all supported combinations that Go provides, only for that repo).

Also a heuristic for a sensible default GOOS and GOARCH could be a good thing. In my case 3 of 30 files are tagged Windows, in this case the site could suggest what it considers the common combination somewhere visible at the top when you go to the default (unspecified GOOS and GOARCH) version of the site?

[jba]

We're actively working on a better UI. It should be out soon.

[gonutz]

Nice, that is what I was guessing from seeing this and other issues and discussions. I just wanted to give my two cents about it. Good job, keep up the good work and I am looking forward to it :-)

[gopherbot]

Change https://golang.org/cl/297550 mentions this issue: internal/frontend: add build context data to MainDetails

[aykevl]

This is trickier to implement than it might seem, because we want to do as much preprocessing as possible on the worker. If we just generated doc from source at serving time, this would be easy, but that's too slow. We would like to at least parse the files on the worker, and generate doc from the ASTs on the frontend.

A .go file with the contents

// +build ignore

Not valid Go code.

is perfectly fine and won't interfere with compiling, unless the ignore build tag is explicitly provided. That means that we can't just parse every file in a package; some might not be valid Go, and that's legal.

Maybe I'm missing something, but what exactly is preventing this? I would imagine it would work like this:

1. All Go files are parsed. Some may have parse errors, that's fine. In that case, instead of the AST a parser error is stored for that file.
2. On load, the set of necessary files is determined and the AST for those files is loaded. If it contains one with a parse error, there is an error on load.
3. Otherwise, if all the files in the set are error-free, the page is rendered based on this set.

I would very much like to see support for Go tags, which would more or less require such a more flexible scheme.

[gopherbot]

Change https://golang.org/cl/297552 mentions this issue: content,internal: add build context dropdown to unit page

[jba]

@aykevl, that's an interesting thought. The problem is that we use go/build.Context.MatchFile to determine the set of files for a build context, which needs to read the source. And we don't want to keep the source around.

We could probably extract the logic of that function and save just the build constraints of each file, but that would be brittle, especially in light of the new build constraint syntax.

[aykevl]

I see, that complicates it a lot. I agree that copying the logic from the build package is way too brittle, there are many special exceptions there.

One hacky workaround would be to save just the first part of all the *.go files (up to the package line, possibly skipping lines that do not start with // +build or //go:build). That still keeps some source around, but only the necessary part. And I think it's far less brittle than replicating the MatchFile logic.

Other than that, I think the only solution would involve a significant refactor in how build tags are handled in the build package. For example, it could provide a method to read the build tags from a Go file in a structure that can be serialized and deserialized. Later (on page load, when the build tags are known) it could use this structure to determine which files to include. Basically, the refactor would need to separate reading the build tags from determining which build tags apply to a given file.

[ianlancetaylor]

The go/build/constraint package, which is new in Go 1.16, can help with some of this.

https://golang.org/pkg/go/build/constraint/

[jba]

We still need a way to go from a go source file to a constraint.Expr. Then we could serialize and store the expression for later evaluation.

Given the structure of the build package, it's not clear where that would live. A build.Context knows how to open files, but it's also tied to a specific set of tags. Those two things would have to be decoupled first.

[aykevl]

The go/build/constraint package, which is new in Go 1.16, can help with some of this.

That's interesting! That would certainly simplify this.

A build.Context knows how to open files, but it's also tied to a specific set of tags.

There is also the UseAllFiles flag, which appears to be for such purposes?

Currently of course the constraint.Expr isn't exposed. But if it is provided after the package is loaded (perhaps as part of build.Package) and is provided as an input to matchFile as an alternative to reading the files, I think it would be possible.

But of course I'm not really involved in this. I would just like to see support for build tags on pkg.go.dev, which are not feasible with the current design.