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: go package readme markdown broken #40203

Closed
Delta456 opened this issue Jul 14, 2020 · 8 comments
Closed

x/pkgsite: go package readme markdown broken #40203

Delta456 opened this issue Jul 14, 2020 · 8 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Milestone

Comments

@Delta456
Copy link

What is the URL of the page with the issue?

https://pkg.go.dev/github.com/Delta456/box-cli-maker?tab=overview

What is your user agent?

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0

Screenshot

image

What did you do?

Update my README.md upon every release

What did you expect to see?

My README.md working like it is https://github.com/Delta456/box-cli-maker/blob/master/README.md

What did you see instead?

README Markdown broken

@gopherbot gopherbot added this to the Unreleased milestone Jul 14, 2020
@andybons
Copy link
Member

@julieqiu

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 14, 2020
@akolar
Copy link

akolar commented Jul 17, 2020

I did some digging into this. It seems that the problem is that blackfriday (markdown rendering engine used by pkgsite) isn't fully compatible with Github-flavoured markdown (GFM).

I can confirm that I wasn't able to (fully) replicate Github's rendering using blackfriday by adding additional format-related flags to https://github.com/golang/pkgsite/blob/master/internal/frontend/overview.go#L143. This seems consistent with the findings in russross/blackfriday#91. In short, their solution to this problem was to create a custom wrapper around blackfriday that is capable of rendering GFM: https://github.com/shurcooL/github_flavored_markdown (note: based on v1 blackfriday, not v2).

Using github_flavored_markdown I was able to render the following readme.

render

The output seems fairly consistent with https://github.com/Delta456/box-cli-maker/blob/master/README.md. There is no code highlighting for snippets and images are missing (can probably be fixed by writing a custom Walk() function similar to the one for blackfriday - didn't try that at this time due to an incompatible blackfriday version).

I think there are a few options for rendering different markdown flavours on pkg.go.dev:

  1. Do something similar to what PyPI is doing (https://packaging.python.org/specifications/core-metadata/#description-content-type-optional) and allow developers to specify the markdown flavour (as a README comment or elsewhere).
  2. "Guess" which flavour to use based on internal.ModuleInfo.SourceInfo.RepoURL() or the import path itself (github.com prefix probably makes it reasonable to assume that GFM should be used).
  3. I think it is not possible to distinguish between different flavours - files might be valid vanilla markdown and valid GFM, but rendered differently.

Between the two (three) alternatives, I think (2) is the most likely to yield desirable results. Using (1) as an override to (2) might also be an option, but it might cause problems with certain markdown flavours (vanilla markdown doesn't support comments, for example. https://daringfireball.net/projects/markdown/syntax). [EDIT: there's obviosuly another option; use GFM for all readmes].

@Delta456
Copy link
Author

@akolar I think everyone prefers GFM so we can go with that.

@akolar
Copy link

akolar commented Jul 18, 2020

Ignore my last message, false positive (github_flavored_markdown uses blackfriday/v1 which works fine). Markdown syntax has nothing to do with.

The issue is with the blackfriday's parser which can't handle CRLF line terminators. This has been reported several times [1, 2, 3, 4] and there's also a PR (WIP) open [5].

To demonstrate what happens on a simplified readme from the OP (with \r, \r\n and \n used as line endings):

package main

import (
	"fmt"
	"log"
	"os"
	"strings"

	"gopkg.in/russross/blackfriday.v2"
)

const input = `
# Heading

- item 1
- item 2

` + "```go" + `
fmt.Println("string", 123) 
` + "```"

func main() {
	f, err := os.Create("out")
	if err != nil {
		log.Fatalf("failed to open out: %v", err)
	}
	defer f.Close()

	for _, nl := range []string{"\r", "\r\n", "\n"} {
		fmt.Fprintf(f, "Using %s:\n", strings.ReplaceAll(strings.ReplaceAll(nl, "\n", `\n`), "\r", `\r`))

		md := strings.ReplaceAll(input, "\n", nl)
		fmt.Fprintf(f, "-- Input:\n%s\n", md)
		f.WriteString("-- Converted:\n")
		f.Write(blackfriday.Run([]byte(md)))
		f.WriteString("\n\n")
	}
}

Output (with displayed whitespace characters):
2020-07-18-191632_796x861_scrot

Removing \r from sources has fixed the issue for me locally. @julieqiu I'll open a CL in a moment and you can take it from there.

[1] russross/blackfriday#423
[2] russross/blackfriday#575
[3] russross/blackfriday#497
[4] russross/blackfriday#394
[5] russross/blackfriday#428

@zikaeroh
Copy link
Contributor

zikaeroh commented Jul 18, 2020

Unless I'm mistaken, blackfriday isn't actively developed anymore, and many have moved onto goldmark. Hugo, gitea, and even x/tools/present and x/website use goldmark for their markdown support.

Perhaps it'd be better to move to a markdown library which is actively maintained.

@gopherbot
Copy link

Change https://golang.org/cl/243457 mentions this issue: internal: replace \r\n with \n in readmes and licences

@Delta456
Copy link
Author

https://pkg.go.dev/github.com/Delta456/box-cli-maker?tab=overview

The code highlighted in ' ' and - are still broken.

@Delta456
Copy link
Author

https://pkg.go.dev/github.com/Delta456/box-cli-maker?tab=overview

The code highlighted in ' ' and - are still broken.

Nvm it got fixed. Thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. pkgsite
Projects
None yet
Development

No branches or pull requests

6 participants