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/tools/gopls: support http:// scheme (insecure) #66389

Open
oliverpool opened this issue Mar 18, 2024 · 12 comments
Open

x/tools/gopls: support http:// scheme (insecure) #66389

oliverpool opened this issue Mar 18, 2024 · 12 comments
Labels
gopls Issues related to the Go language server, gopls. help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@oliverpool
Copy link

oliverpool commented Mar 18, 2024

gopls version

0.15.2-1 (on arch linux)

go env

(not relevant, I believe)

GO111MODULE=''
GOARCH='amd64'
GOPRIVATE=''
GOVERSION='go1.22.1'

What did you do?

I have been developing vanitydoc to be able to generate simple HTML documentation at the import path of my go libraries.

This library is also able to generate those files "on-the-fly", for instance running the following command serves the documentation of the standard library, as well as the documentation of the packages in my gomodcache locally:

vanitydoc -gomodcache=$(go env GOMODCACHE) /usr/lib/go/src

I have setup a service file to start this automatically under localhost: http://localhost:8080 (actually I am using http://pkg/) and I ajusted the gopls settings inside vscode to: "ui.documentation.linkTarget": "http://pkg"

What did you see happen?

When clicking on a doc link, the https:// gets prepended: https://http://pkg/src.agwa.name/go-listener@v0.5.1#Open

What did you expect to see?

I would expect to be able to visit the http version: http://pkg/src.agwa.name/go-listener@v0.5.1#Open

Editor and settings

settings.json of vscode:

"gopls": {
    "ui.documentation.linkTarget": "http://pkg"
  }

Possible fix

Currently the s of https is hard-coded msg := fmt.Sprintf("https://%s/%s", options.LinkTarget, link.ImportPath)
https://github.com/golang/tools/blob/c21ae4cabc44594caf9cbfa867a678ad4a2df387/gopls/internal/golang/comment.go#L29

I would propose to move the logic to settings.go:
https://github.com/golang/tools/blob/2d517d51b81102672a95b40c9fea60a65ba3620a/gopls/internal/settings/settings.go#L985

  • if the variable is empty, don't do anything
  • If no scheme is set, set it to https
  • if a scheme is set, use it as-is
@oliverpool oliverpool added gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. labels Mar 18, 2024
@gopherbot gopherbot added this to the Unreleased milestone Mar 18, 2024
@oliverpool
Copy link
Author

@hyangah looking through the gopls issues about this setting, I read #51848 (comment)

vscode-go (&gopls) plans to do it eventually - we prefer more simplified documentation (either a plain html or markdown without special styling/javascript) than what pkg.go.dev shows and want ability to get the documentation part only so the editor can choose the right style (e.g. dark theme, ... )

Feel free to give a try to https://code.pfad.fr/vanitydoc/ (it is quite new and maybe bit rough). Its original goal is to generate static HTML for hosting at a vanity-URL, however it serves quite well as a local server of documentation. To serve the module in the current folder, run:

go run code.pfad.fr/vanitydoc@latest .

@findleyr
Copy link
Contributor

CC @adonovan, who has recently integrated a simple documentation viewer into gopls, and may find this interesting.

The behavior of linktarget seems like a bug. It should be possible to use a local http server. Perhaps we can avoid prepending a scheme if the linktarget already has a scheme.

Tentatively putting this in the gopls@v0.16.0 milestone, and marking as help wanted.
The fix would be to look for uses of settings.Options.LinkTarget, and fix places (such as gopls/internal/cache.BuildLink) where we prepend https. We should only prepend https if the linktarget does not already have a scheme.
See gopls/internal/test/marker/testdata/hover for example tests.

@oliverpool
Copy link
Author

Looking at the codebase, there are 2 conditions if strings.ToLower(options.LinkTarget) == "pkg.go.dev" {, to append the version info to the URL.

I think it would be nice to fix those as well (vanitydoc supports URLs with version info). For instance by adding 2 fields LinkTargetScheme and LinkTargetWithModuleVersion to DocumentationOptions:

type DocumentationOptions struct {
	// HoverKind controls the information that appears in the hover text.
	// SingleLine and Structured are intended for use only by authors of editor plugins.
	HoverKind HoverKind

	// LinkTarget controls where documentation links go.
	// It might be one of:
	//
	// * `"godoc.org"`
	// * `"pkg.go.dev"`
	//
	// If company chooses to use its own `godoc.org`, its address can be used as well.
	//
	// Modules matching the GOPRIVATE environment variable will not have
	// documentation links in hover.
	LinkTarget string

	// LinkTargetScheme controls the documentation link scheme.
	// "https://" by default
	LinkTargetScheme string

	// LinkTargetWithModuleVersion controls addition of the module version to the documentation URL.
	// If set to "true" (by default when LinkTarget is "pkg.go.dev", false otherwise),
	// the version will be appended to the module URL,
	// e.g. pkg.go.dev/example.com/module@v1.0/package instead of godoc.org/example.com/module/package
	LinkTargetWithModuleVersion bool

	// LinksInHover toggles the presence of links to documentation in hover.
	LinksInHover bool
}

@adonovan
Copy link
Member

Hi Oliver, thanks for your interest in improving gopls' documentation rendering. Please build gopls at master and try out the new "Source Action > View package documentation" feature in VS Code (or your editor's equivalent); I think we'll find we're working towards a common purpose. I haven't looked at vanitydoc yet, but perhaps its features can be incorporated directly into gopls, making it easier for users to discover and use.

I agree the LinkTarget field should be able to select insecure HTTP. We may need to permit an optional http: or https: prefix on it. (See also https://go.dev/cl/572037/1, in which the special value "gopls" enables use of gopls' new internal web server.)

@oliverpool
Copy link
Author

try out the new "Source Action > View package documentation" feature

This look neat! Especially the "click to open back in vscode"!

I got a panic when trying to view the documentation of example/nested/chan.go from the https://codeberg.org/pfad.fr/vanitydoc codebase (this is a submodule with an import missing from go.mod).

Do you have any document indicating the state of this viewer and its goal? Or is it only scattered as TODO(...) through the codebase?
From a quick glance, I noticed the following:

I haven't looked at vanitydoc yet

The pages under https://code.pfad.fr/vanitydoc/ have been rendered with vanitydoc. The original goal was to generate static websites, and then I realized that I could that locally on the fly from the gomodcache.

For vanitydoc it would be greatly beneficial to have an importable package able to produce HTML documentation for chunks of ast (with control regarding the generated links), because escaping HTML and navigating the ast is quite cumbersome: https://codeberg.org/pfad.fr/vanitydoc/src/branch/main/template/ast.go (might not be accessible due to scheduled maintenance)

@oliverpool
Copy link
Author

And what to you think of the proposition to add the LinkTargetScheme and LinkTargetWithModuleVersion fields to DocumentationOptions? (or should this be dropped in favor of the integrated viewer?)

@adonovan
Copy link
Member

This look neat! Especially the "click to open back in vscode"!

Thanks!

I got a panic when trying to view the documentation of example/nested/chan.go from the https://codeberg.org/pfad.fr/vanitydoc codebase (this is a submodule with an import missing from go.mod).

Thanks, I can reproduce it. I have filed it as #66449.

Do you have any document indicating the state of this viewer and its goal? Or is it only scattered as TODO(...) through the codebase? From a quick glance, I noticed the following:

No, sorry; the scattered TODOs are the only record.

All of these are indeed missing features, and we would welcome CLs to address any of them.

For vanitydoc it would be greatly beneficial to have an importable package able to produce HTML documentation for chunks of ast (with control regarding the generated links), because escaping HTML and navigating the ast is quite cumbersome: https://codeberg.org/pfad.fr/vanitydoc/src/branch/main/template/ast.go (might not be accessible due to scheduled maintenance)

In earlier discussions about this new feature, I too thought it would have been greatly beneficial to have an importable package able to produce HTML documentation for chunks of ast (it is indeed cumbersome), and was hoping to extract parts of pkgsite to achieve that. But eventually I realized that the representation of the package is so different (and richer) in gopls that it would be counterproductive to try to factor most of the rendering. But I do think we should try to share (or steal) the tricky syntax-to-HTML logic. (The old cmd/godoc contains one or two similar algorithms too.)

@adonovan
Copy link
Member

adonovan commented Mar 21, 2024

And what to you think of the proposition to add the LinkTargetScheme and LinkTargetWithModuleVersion fields to DocumentationOptions? (or should this be dropped in favor of the integrated viewer?)

I don't think splitting the URL into such small parts is the way to go. I think it would be simpler to redefine the existing single string so that is interpreted as a URL prefix, onto which a suffix like /pkg/fmt#Println should be appended. The scheme prefix should be optional, and default to "https:" if unspecified. Certain special values such as "gopls" (for the internal viewer) can be carved out too.

@oliverpool
Copy link
Author

There are 2 distinct issues.

Ability to specify the scheme

Instead of having an additional parameter, the scheme could be automatically added if missing:

  • pkg.go.dev => https://pkg.go.dev
  • http://pkg => http://pkg

This one can be "easily" managed without adding a parameter

Does the documentation support @version suffix

pkg.go.dev support both

However gddo supports only URLs without a version (as far as I know).

Godocs.io and vanitydoc (local server) both supports the version suffix:

This could be hacked into the url-setting by using an anchor:

  • http://godoc.internal#no-version <= no @version suffix should be added to documentation links
  • http://pkg <= by default, the @version suffix should be added to documentation links

Having a setting for the symbol anchor #Println is not really important since it will not trigger any error for the user (the page will just not jump to the definition).

@adonovan
Copy link
Member

Ability to specify the schema

I agree that adding an https:// prefix only if no scheme is already present seems reasonable.

Does the documentation support @ version suffix

Perhaps a more general approach is needed, supporting template strings such as the following:

"https://pkg.go.dev/{package}@{version}"
"https://godocs.io/{package}@{version}"
"http://localhost:8080/{package}"
"gopls"  // carve-out for internal server

For compatibility, existing strings that don't contain "/{package}" would behave as if they had that added as a suffix.

(Perhaps the @ should be part of the expansion of version, since the version is not always known, and we don't want a trailing @ in that case.)

I have setup a service file to start this automatically under localhost: http://localhost:8080 (actually I am using http://pkg/)

Just curious: how did you arrange for the local host name pkg to specify a non-default port?

@oliverpool
Copy link
Author

Sounds good!

I would suggest {@version}, to make it clear that the @ will be automatically prepended to the version (and if needed a {version} could be added later if some system expect no leading @):

https://pkg.go.dev/{package}{@version}

how did you arrange for the local host name pkg to specify a non-default port?

I stripped a lot of information with my parenthesis 🙈, here is my actual setup:

  • I added a 127.0.103.111 pkg entry to my hosts file
  • I created a systemd vanitydoc.socket file with ListenStream = 127.0.103.111:80 (needs elevated permissions)
  • The vanitydoc.service file then forward the connection via a file descriptor vanitydoc -addr fd:3 -gomodcache=... (does not need permission)

I documented this today on my blog https://log.pfad.fr/2024/vanitydoc-offloaded/


I would love to make some CLs, however I am currently spending way too much time on unpaid (open-source) work and need some freelance-contract first 😆

@oliverpool
Copy link
Author

Actually https://pkg.go.dev/{package}{@version} is wrong. It is more like https://pkg.go.dev/{module}{@version}{package} and https://godoc.org/{module}{/package} (the version appears between the module name and the package - if any)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. help wanted Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants