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

Open
ChrisHines opened this issue Feb 14, 2020 · 33 comments
Assignees

Comments

@ChrisHines
Copy link
Contributor

@ChrisHines ChrisHines commented Feb 14, 2020

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 dmitshur added the pkgsite label Feb 14, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 14, 2020

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 removed the Documentation label Feb 14, 2020
@dolmen
Copy link

@dolmen 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 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 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
Member

@bcmills 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 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 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

@gopherbot gopherbot commented Oct 1, 2020

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

@firelizzard18 firelizzard18 commented Nov 11, 2020

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

@gopherbot gopherbot commented Dec 21, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 21, 2020

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

@gopherbot gopherbot commented Feb 4, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 4, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 4, 2021

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

gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2021
Improve the control flow when loadPackageWithBuildContext
returns an error. List the cases clearly.

For golang/go#37232

Change-Id: I09d7b1abfc4e933c540e5f7df8b715219db12457
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/289672
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2021
loadPackageWithBuildContext now returns only build-context-specific
values, not an entire goPackage. Instead, the goPackage is assembled
inside the loop over build contexts in loadPackage. This clarifies
the package-building logic and allows us to delete mergePackages.

For golang/go#37232

Change-Id: Ia46ca309f0ad30e30ba6f8dd8f8e797139d7399a
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/289673
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2021
Hoist matching of files to build context up to the loop over build
contexts in loadPackages. This will enable us to detect identical sets
of files early, avoiding unnecessary work and reducing duplication of
documentation.

For golang/go#37232

Change-Id: I90e236cf0c72b81500f7134bb15d710c92e0f1bc
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/289674
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2021
Remember the set of files in a build context, to avoid loading
the same files repeatedly.

For golang/go#37232

Change-Id: I533f7de1313057ba79b1d1a6703c07ae0d4d8078
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/289675
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 4, 2021
Previously, the serialized source used for documentation included the
GOOS/GOARCH of the build context. This was redundant, since the
internal.Documentation that contains it already has GOOS/GOARCH
information. It is now also misleading, because we may reuse source
across build contexts if the set of matched files is the same.
So use empty strings for GOOS/GOARCH instead.

For golang/go#37232

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

@gopherbot gopherbot commented Feb 4, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 4, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 5, 2021

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

gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 5, 2021
As argued in the previous CL (https://golang.org/cl/c/289676), GOOS
and GOARCH shouldn't be stored in the encoded source. Instead of
leaving them empty, remove them entirely.

The codec logic will skip those fields when it encounters them in
existing serialized data.

For golang/go#37232

Change-Id: Ia5066f71fe245d7f96a7289a9a086c6bc7959a75
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/289678
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 5, 2021
The documentation table has obsolete rows now that we are using
all/all to represent documentation for all build contexts.
So remove existing rows for a unit before inserting new ones.

For golang/go#37232

Change-Id: Icee1457ecc1e336c42e66e16d58a1f802a6c95f6
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/289989
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
Copy link

@gopherbot gopherbot commented Feb 5, 2021

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

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 5, 2021

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

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 label Feb 12, 2021
@jba
Copy link
Contributor

@jba 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 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

@firelizzard18 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 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 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 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 jba commented Feb 25, 2021

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

@gonutz
Copy link

@gonutz 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

@gopherbot gopherbot commented Mar 1, 2021

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet