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: request for documentation for non-std modules responds with an error #51368

Closed
bhcleek opened this issue Feb 25, 2022 · 5 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite/cmd Issues related to x/pkgsite/cmd/pkgsite pkgsite
Milestone

Comments

@bhcleek
Copy link
Contributor

bhcleek commented Feb 25, 2022

What is the URL of the page with the issue?

Any module that does not have a dot in its name and is not part of the standard library.

What is your user agent?

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/98.0.4758.102 Safari/537.36

What did you do?

  1. Run a local pkgsite instance to serve documentation for a module that is only available internally and which does not have a . in its name.
  2. Request the godoc for module's packages.

What did you expect to see?

The godoc for the page.

What did you see instead?

An error page.

The issue is caused by "github.com/golang/pkgsite/internal/frontend".serveDetails, which calls "github.com/golang/pkgsite/internal/frontend".extractURLPathInfo which in turn relies on "github.com/golang/pkgsite/internal/stdlib".Contains.

While I can understand the standard library taking precedence, it would be nice if serveDetails would fallback to checking the other modules so that modules which are internal to a company and not expected to be available publicly could have documentation served by pkgsite as well.

Unfortunately, updating the module name is a disruption that is not yet worth the trouble.

@gopherbot gopherbot added this to the pkgsite/unplanned milestone Feb 25, 2022
@jamalc
Copy link

jamalc commented Mar 14, 2022

I understand the use case but I'm not sure this is something we would move forward with. Package names should always be globally unique and this namespace is reserved for the stdlib. Others might disagree. /cc @jba @julieqiu

Would the replace directive be useful here in avoiding major disruption from a module name change?

@jamalc jamalc added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 14, 2022
@jamalc jamalc modified the milestones: pkgsite/unplanned, pkgsite/cmd Mar 14, 2022
@jba
Copy link
Contributor

jba commented Mar 14, 2022

I agree with Jamal.
If you don't want to change your package names, I'd suggest patching our code for your local deployment.

@bhcleek
Copy link
Contributor Author

bhcleek commented Mar 14, 2022

this namespace is reserved for the stdlib

I recognize that this is understood within the Go team, and I understand why packages without a dot in the name must be preferred to come from stdlib in the event of a collision, but is this documented some place? Since starting to work with Go in 2013, I have never seen anyone say this or talk about it except for in some GitHub issues and some meetings with the Go team.

Given that godoc doesn't have this restriction and given that the go tool itself doesn't enforce such a restriction on package names, the incongruence in pkgsite is very unexpected.

@hyangah hyangah added the pkgsite/cmd Issues related to x/pkgsite/cmd/pkgsite label May 20, 2022
@seankhliao
Copy link
Member

is this documented some place

That would be #32819

Closing as this isn't going to be supported

@Sunshine40
Copy link

Sunshine40 commented Dec 11, 2022

@seankhliao The reasons for not lifting the ristrictions here is fair enough, but the error message returned should be revised.

The HTTP 424 Failed Dependency along with the message "This page is not supported by this datasource." is exceptionally vague to use as a search engine keyword, and is actually misleading in this case.

As @julieqiu mentioned in #40371, pkgsite (run locally as a light-weight go doc visualizer for private repositories) can be used as an alternative to the godoc cmd which also serves http requests, so running into this problem isn't that rare.

After hours of digging into the source code of pkgsite, I was finally confirmed that the error page has nothing to do with the datasource choice (of not using postgresql). The error handling logic redirects every 404 not found to this 424 failed dependency if the datasource is not postgresql, which is actually counterintuitive as 404s are most certainly 404s.

To better explain: error handler tried to tell user that a 404 not found is due to local filesystem as datasource, mistaking the fact for the actual cause when 404 occured. This logic is flawed for queries that normally return 404s from a postgresql backed instance of pkgsite.

If someone made his first attempt to run pkgsite locally with a single-word named module, and ended up with the program producing "not supported by datasource" page for every url he could come up with, while developer mentioned only

Search and other tabs on the package page are not supported in direct proxy mode.

he would expect the problem to be improper deployment, and head to the wrong direction. (I wasted hours ruling out the possibilities of "needing a postgresql server to fix", "undocumented dependency on linux environment while I run windows", "unknown problem connecting to database", "go for windows not recognizing user-scoped environment variables", definitely don't want to be repeated)

Limiting the "not supported by datasource" error page to search queries should help. And it would be nice to have a link guiding newcomers to the documentation about #32819

@golang golang locked and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite/cmd Issues related to x/pkgsite/cmd/pkgsite pkgsite
Projects
None yet
Development

No branches or pull requests

7 participants