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

Comments

@Delta456
Copy link

@Delta456 Delta456 commented Jul 14, 2020

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

@andybons andybons commented Jul 14, 2020

@akolar
Copy link

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

@Delta456 Delta456 commented Jul 18, 2020

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

@akolar
Copy link

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

@gopherbot gopherbot commented Jul 18, 2020

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

@Delta456
Copy link
Author

@Delta456 Delta456 commented Jul 21, 2020

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

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

@Delta456
Copy link
Author

@Delta456 Delta456 commented Jul 24, 2020

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 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
6 participants