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

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

Closed
ChrisHines opened this issue Feb 14, 2020 · 40 comments
Labels
FeatureRequest FrozenDueToAge pkgsite/dochtml Issues related to package documentation in pkgsite pkgsite UX Issues that involve UXD/UXR input

Comments

@ChrisHines
Copy link
Contributor

What did you do?

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
Copy link
Contributor

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

/cc @julieqiu @dmitshur

@dmitshur dmitshur added this to the Unreleased milestone Feb 14, 2020
@dmitshur dmitshur changed the title pkg.go.dev: Support showing GOOS and GOARCH specific docs. go.dev: support different GOOS/GOARCH values when displaying package documentation Feb 14, 2020
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Documentation labels Feb 14, 2020
@dolmen
Copy link
Contributor

dolmen commented Mar 20, 2020

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

@julieqiu julieqiu added the UX Issues that involve UXD/UXR input label Apr 22, 2020
@julieqiu julieqiu changed the title go.dev: support different GOOS/GOARCH values when displaying package documentation x/pkgsite: support different GOOS/GOARCH values when displaying package documentation Jun 15, 2020
@julieqiu julieqiu modified the milestones: Unreleased, pkgsite/dochtml Aug 19, 2020
@jba
Copy link
Contributor

jba commented Oct 1, 2020

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
Copy link
Contributor

bcmills commented Oct 1, 2020

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
Copy link
Member

myitcv commented Oct 1, 2020

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
Copy link
Member

mvdan commented Oct 1, 2020

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 #39005.

@jba jba self-assigned this Oct 1, 2020
@gopherbot
Copy link

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

gopherbot pushed a commit to golang/pkgsite that referenced this issue Oct 1, 2020
Package records the GOOS/GOARCH combination it was created with;
Render checks it.

Someday we'll allow multiple combos, but it requires a bit of
refactoring in internal/fetch, and could conceivably cause us to fail
on packages that we now succeed on (by parsing a file from a build
context that we don't currently get to). So we'll leave it for later.
See golang/go#37232.

Change-Id: I3d5e608fefebf4a9e825d1f8f8a5d1166b7e573b
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/258737
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
@firelizzard18
Copy link
Contributor

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
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit to golang/pkgsite that referenced this issue Dec 22, 2020
loadPackage has two return values: a *goPackage and an error.

Before this CL, if a package's documentation was too large,
loadPackage would return the package and a non-nil error.

With this CL, that error is stored in the goPackage struct in a new
field, and loadPackage returns a nil error.

Aside from being more idiomatic, this change will enable future
changes needed to support multiple GOOS/GOARCH combinations.

For golang/go#37232

Change-Id: If73adde76239046eb22409659d6795c09bbb1fd4
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/279457
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 8, 2021
Handle BuildContexts where one or more parts are "all".

These should only occur when searching for a matching Documentation,
not sorting. (Because we sort only when there is more than one
Documentation, and if there is an all/all Documentation then there
aren't any others.)

For golang/go#37232

Change-Id: I898aba8f73d2682798d56c47cc6773709f10c702
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/290094
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 8, 2021
If all the build contexts for a package produce the same documentation,
then return a single Documentation with build context all/all.

This will save a lot of space in the documentation table, since
most packages will fit into this case.

For golang/go#37232

Change-Id: I15237242e0c3ca3c7b8f8c8944b227afebd23785
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/289680
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 8, 2021
If it is not all/all, show the build context at
the bottom of the documentation.

This is just a preliminary implementation, so users
can experience the feature before we provide a proper UI.

For golang/go#37232

Change-Id: Ic48a246a8793e61917a31f66580249997255ba77
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/290095
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
@jba jba removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 12, 2021
@jba
Copy link
Contributor

jba commented Feb 12, 2021

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
Copy link
Contributor

tianon commented Feb 12, 2021

  • 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
Copy link
Contributor

firelizzard18 commented Feb 12, 2021

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.

image

@jba
Copy link
Contributor

jba commented Feb 12, 2021

@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
Copy link
Contributor

jba commented Feb 12, 2021

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

@gonutz
Copy link

gonutz commented Feb 25, 2021

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
Copy link
Contributor

jba commented Feb 25, 2021

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

@gonutz
Copy link

gonutz commented Feb 25, 2021

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
Copy link

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

gopherbot pushed a commit to golang/pkgsite that referenced this issue Mar 1, 2021
Adds documentation build context data to support the build context
dropdown on the doc pages.

For golang/go#37232

Change-Id: I8ff333f811612b89db95064cf387b12e0316ed13
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/297550
Trust: Julie Qiu <julie@golang.org>
Trust: Jamal Carvalho <jamal@golang.org>
Run-TryBot: Julie Qiu <julie@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
TryBot-Result: kokoro <noreply+kokoro@google.com>
@aykevl
Copy link

aykevl commented Mar 10, 2021

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
Copy link

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

@jba
Copy link
Contributor

jba commented Mar 22, 2021

@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
Copy link

aykevl commented Mar 23, 2021

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
Copy link
Contributor

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
Copy link
Contributor

jba commented Mar 23, 2021

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
Copy link

aykevl commented Mar 23, 2021

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.

@golang golang locked and limited conversation to collaborators Mar 23, 2022
@hyangah hyangah added the pkgsite/dochtml Issues related to package documentation in pkgsite label May 20, 2022
@rsc rsc unassigned jamalc and jba Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge pkgsite/dochtml Issues related to package documentation in pkgsite pkgsite UX Issues that involve UXD/UXR input
Projects
None yet
Development

No branches or pull requests