Skip to content

x/pkgsite: improve pkgsite command #47780

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.

Sign up for GitHub

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

Jump to bottom
Closed
2 tasks done
jba opened this issue Aug 18, 2021 · 40 comments
Closed
2 tasks done

x/pkgsite: improve pkgsite command #47780

jba opened this issue Aug 18, 2021 · 40 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Milestone
pkgsite/unplanned

Comments

@jba
Copy link
Contributor

jba commented Aug 18, 2021 •
edited
Loading

Improve golang.org/x/pkgsite/cmd/pkgsite so it can be used instead of golang.org/x/tools/cmd/godoc.

  • Serve modules from the module cache.
  • Serve modules directly from the proxy.
@jba jba added the pkgsite label Aug 18, 2021
@jba jba added this to the pkgsite/unplanned milestone Aug 18, 2021
@jba jba self-assigned this Aug 18, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/343211 mentions this issue: internal/fetch: test local fetching with empty module path

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/343210 mentions this issue: internal/fetch: add ModuleGetter interface

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/343212 mentions this issue: internal/fetch: use a ModuleGetter for FetchLocalModule

@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 18, 2021
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 18, 2021
@jba
internal/fetch: add ModuleGetter interface
1ab18f2 
Change-Id: Ie2495949a365f5cbdfcfdc42c12233348c88b85c

For golang/go#47780

Change-Id: Ibe93986002ffa4f0a5419cf7ce7ce0987fa95cb2
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/343210
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 Aug 18, 2021
@jba
internal/fetch: test local fetching with empty module path
4a242f5 
For golang/go#47780

Change-Id: Ib32d9b56540ce6786e5951829fcf2b8999303057
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/343211
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
Contributor

Change https://golang.org/cl/343220 mentions this issue: internal/fetch: add a ModuleGetter for a proxy-like filesystem

gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 18, 2021
@jba
internal/fetch: use a ModuleGetter for FetchLocalModule
5b6aca9 
For golang/go#47780

Change-Id: I1259649c8a1d452ffad5e8a2a92943801a24b306
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/343212
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 Aug 18, 2021
@jba
internal/fetch: add a ModuleGetter for a proxy-like filesystem
af3438a 
Add fsModuleGetter, which is a ModuleGetter for a directory organized
like proxy URLs.  The go module download cache is in this format.

For golang/go#47780

Change-Id: Ifccdfa3010874adddc0a600593699d454e44c0e3
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/343220
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
Contributor

Change https://golang.org/cl/343591 mentions this issue: internal/localdatasource: use getters

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/343590 mentions this issue: internal/fetch: a directoryModuleGetter knows its module path

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/343629 mentions this issue: internal/fetch: remove FetchLocalModule

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/343630 mentions this issue: cmd/pkgsite: add a test

gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 19, 2021
@jba
internal/fetch: a directoryModuleGetter knows its module path
f4073fd 
A correctly configured directoryModuleGetter knows the module path it
is serving, and returns NotFound for any others.

This will enable trying multiple ModuleGetters when looking for a
path.

For golang/go#47780

Change-Id: Ib33bd3d7e222ff048ae8a35843df43219e9d844b
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/343590
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 Aug 19, 2021
@jba
internal/localdatasource: use getters
5e769f6 
Instead of loading a list of modules initially, a local datasource now
takes a list of ModuleGetters, which are called on demand when a
module is requested.

For golang/go#47780

Change-Id: Ica3cc6d47de01ec78c451c0ef54b9ba0e0c5a96e
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/343591
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 Aug 19, 2021
@jba
internal/fetch: remove FetchLocalModule
34219a3 
It is no longer used.

For golang/go#47780

Change-Id: I9857e6b4d0ac77cfe4b30ce02e670fc93f34e51e
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/343629
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 Aug 20, 2021
@jba
cmd/pkgsite: add a test
a0baa6d 
Add a small test to the pkgsite command to make sure it's working.

For golang/go#47780

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

so it can be used instead of golang.org/x/tools/cmd/godoc

Please do not remove or deprecate godoc 🙏

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/344529 mentions this issue: internal/localdatasource: use CandidateModulePaths

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/344530 mentions this issue: internal/datasource: combine datasource packages

@scott-cotton
Copy link

so it can be used instead of golang.org/x/tools/cmd/godoc

Please do not remove or deprecate godoc 🙏

godoc still has all the -analysis flags. Does pkgsite offer that?

@jba
Copy link
Contributor Author

jba commented Aug 23, 2021

godoc still has all the -analysis flags. Does pkgsite offer that?

No.

cmd/pkgsite will just serve documentation from the local filesystem and, optionally, directly from the proxy. It won't have all the features of godoc. We won't remove golang.org/x/tools/cmd/godoc; feel free to use it as long as you wish.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/344589 mentions this issue: internal/datasource: factor out cache

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/344590 mentions this issue: internal/datasource: limit cache size

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/344669 mentions this issue: internal/datasource: factor out getters

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/344670 mentions this issue: internal/fetch: populate Unit.BuildContexts

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/344671 mentions this issue: internal/datasource: don't set Unit.BuildContexts

gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 25, 2021
@jba
internal/datasource: factor out getModule
e71d1f3 
Move the getModule method into the shared implementation.

Move the proxy client into the shared implementation, and set it to
nil for the local proxy.

Remove the lock around getModule. As the comment explains, the
resulting race is benign, and we gain the benefit of allowing multiple
goroutines to look up modules concurrently.

Remove the BuildContext arg to getModule. It's not appropriate there,
since getModule's job is to deliver all the information about a module.
We'll re-introduce it (and use it properly) later.

For golang/go#47780

Change-Id: I3e9440c0d6c1b24f7a190a516c9efac1ec0f05bd
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/344949
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 25, 2021
@jba
internal/datasource: factor out GetUnit and GetUnitMeta
d2458ea 
Move both methods into the shared datasource, and clean up the logic.

For golang/go#47780

Change-Id: I7ce0d29742652c63db2d2e55dedf6e378b838445
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/344950
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 25, 2021
@jba
internal/datasource: factor out everything else
ec70ff3 
Move GetLatestInfo to the shared datasource.

Remove GetModuleInfo; it was unused.

For golang/go#47780

Change-Id: I94419458f219759f83c990124d31fcfbddbb4b8e
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/344951
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 Aug 25, 2021
@jba
internal/datasource: consolidate
3f3c89d 
Merge ProxyDataSource and LocalDataSource into a single type,
DataSource.

Combine and simplify tests.

For golang/go#47780

Change-Id: I3510fee3a3d786705a2306a6233b352e5af40076
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/344952
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 Aug 25, 2021
@jba
internal/fetchdatasource: rename from datasource
f56f873 
Rename the package and type by including the word "fetch," to describe
what it does and distinguish it from other DataSources.

For golang/go#47780

Change-Id: I7c674d8ba3dc16084a857039b0cc2b3147f27a29
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/344953
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
Contributor

Change https://golang.org/cl/345249 mentions this issue: fetchdatasource: ignore latest-version errors

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/345251 mentions this issue: internal/fetchdatasource: GetUnitMeta returns NotFound on missing package

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/345269 mentions this issue: internal/fetchdatasource: handle build contexts properly

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/345270 mentions this issue: cmd/pkgsite: accept space-separated paths too

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/345271 mentions this issue: cmd/pkgsite: support proxy

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/345272 mentions this issue: cmd/pkgsite: support the module cache

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/345273 mentions this issue: internal/fetch: support latest version in modcache getter

gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 26, 2021
@jba
internal/fetchdatasource: ignore latest-version errors
ac26f27 
A FetchDataSource may be configured with a proxy for latest-version
information while still being used for local modules. So ignore
latest-version errors.

For golang/go#47780

Change-Id: I30448e133faec80002194df59e6f2db449f80625
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/345249
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 26, 2021
@jba
internal/fetchdatasource: GetUnitMeta returns NotFound on missing pac…
0eeccf0 
…kage

If a module exists but the package path is not in it, return NotFound.

For golang/go#47780

Change-Id: If3a6602df4b99c8470020e8538e01c685880d86d
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/345251
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 26, 2021
@jba
internal/fetchdatasource: handle build contexts properly
f6f5076 
GetUnit returns a Unit with its Documentation set to a matching
BuildContext.  This is the same behavior as postgres.DB.GetUnit.

For golang/go#47780

Change-Id: I2bc23b7bc5a006e78bec54f6f3229e59ab5a03ef
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/345269
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 26, 2021
@jba
cmd/pkgsite: accept space-separated paths too
59bb0ed 
Treat any command-line argument as a path or comma-separated list of
paths.

For golang/go#47780

Change-Id: I5d9044d3ff0e14af99c4632f3c723039179a94a5
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/345270
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 26, 2021
@jba
cmd/pkgsite: support proxy
64e62fe 
If the -proxy flag is provided, the pkgsite command will fetch modules
from the proxy if they can't be found locally.

For golang/go#47780

Change-Id: I02b9fddac9013d9d3859b78e6c8887282469c9a8
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/345271
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 26, 2021
@jba
cmd/pkgsite: support the module cache
925b82e 
With the -cache flag, fetch modules from the module cache.

For golang/go#47780

Change-Id: I2aa6467955cd90a80ccae559f27928cca6e06079
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/345272
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Aug 26, 2021
@jba
internal/fetch: support latest version in modcache getter
eea9463 
If the fsProxyModuleGetter is asked for the latest version, it finds
it by looking at the versions of all the cached zips for the module.

For golang/go#47780

Change-Id: I6e0b1f51c994e99bbcf3e66f40ef4c504fe48ce8
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/345273
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
@jba
Copy link
Contributor Author

jba commented Aug 27, 2021

This work is basically done at master. You can now run cmd/pkgsite with the -cache flag to serve from your local module cache, and with the -proxy flag to serve from the proxy.

The pages served by cmd/pkgsite are not as feature-rich as their pkg.go.dev counterparts, and never will be. But they can probably be improved. For one thing, source links are broken; I filed #47982 for that.

@fzipp
Copy link
Contributor

fzipp commented Aug 27, 2021

@jba I just tried pkgsite -cache, and it worked. I have two questions:

  1. I have to run it from the pkgsite project directory, because the static assets are not embedded in the binary, correct? (The error message when run from another directory wasn't very helpful: Error: cannot obtain module path for ".")
  2. Search does not work (yet?), I have to navigate to the module documentation by editing the URL, or is there another way?

@fzipp
Copy link
Contributor

fzipp commented Aug 27, 2021

Also, the standard library packages (/std and the like) aren't served.

@jba
Copy link
Contributor Author

jba commented Aug 27, 2021

I have to run it from the pkgsite project directory, because the static assets are not embedded in the binary

They aren't embedded, but you can use the -static flag to specify their location. I just sent https://go-review.googlesource.com/c/pkgsite/+/345690, which clarifies that in the doc comment and makes some other changes which should help the ergonomics a bit.

Search does not work (yet?)

Sadly, search will never work, because it relies on information in the database. To get search, you would have to run your own full system, with a frontend, a worker, and a shared DB, and process the modules you wanted to search.

the standard library packages aren't served

They are, it just takes a while for the first package (about a minute on my machine) because it has to clone the go repo and process it. After that, it's cached, so pretty fast.

@fzipp
Copy link
Contributor

fzipp commented Aug 27, 2021

They aren't embedded, but you can use the -static flag to specify their location. I just sent https://go-review.googlesource.com/c/pkgsite/+/345690, which clarifies that in the doc comment and makes some other changes which should help the ergonomics a bit.

Thanks.

Sadly, search will never work, because it relies on information in the database. To get search, you would have to run your own full system, with a frontend, a worker, and a shared DB, and process the modules you wanted to search.

An alternative could be an index page listing all the available modules from the cache.

They are, it just takes a while for the first package (about a minute on my machine) because it has to clone the go repo and process it. After that, it's cached, so pretty fast.

Ah, ok. The concrete standard library package pages like /net/http work after a while, but the /std page stays empty below the "Details" area and the "Jump to" box.

Thanks for all the work.

@jba
Copy link
Contributor Author

jba commented Sep 7, 2021

An alternative could be an index page listing all the available modules from the cache.

They are, it just takes a while for the first package (about a minute on my machine) because it has to clone the go repo and process it. After that, it's cached, so pretty fast.

Just added this: if you clone the go repo and point to it with the -gorepo flag, standard library pages will render slightly faster (not a lot faster).

@jba
Copy link
Contributor Author

jba commented Sep 7, 2021

Since #47982 is done, I'm going to close this.

@jba jba closed this as completed Sep 7, 2021
@rsc rsc unassigned jba Jun 23, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Projects
None yet
Milestone
pkgsite/unplanned
Development

No branches or pull requests
6 participants
@peterbourgon @fzipp @julieqiu @gopherbot @jba @scott-cotton