From 288c39643906b4194a0a6acfbaf87cb0fbdeb361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Erik=20Pedersen?= Date: Tue, 24 Apr 2018 05:57:33 +0200 Subject: [PATCH] hugolib: Fix some shortcode vs .Content corner cases This is a follow-up to #4632. There were some assumptions in that implementation that did not hold water in all situations. This commit simplifies the content lazy initalization making it more robust. Fixes #4664 --- hugolib/hugo_sites.go | 33 +--------- hugolib/hugo_sites_build.go | 17 +++++- hugolib/page.go | 77 +++++++++++------------- hugolib/pageSort_test.go | 8 ++- hugolib/page_test.go | 14 ++--- hugolib/shortcode.go | 15 ++++- hugolib/shortcode_test.go | 116 +++++++++++++++++++++++++++++------- hugolib/site.go | 4 +- 8 files changed, 175 insertions(+), 109 deletions(-) diff --git a/hugolib/hugo_sites.go b/hugolib/hugo_sites.go index e375b0eba..a5bb664c0 100644 --- a/hugolib/hugo_sites.go +++ b/hugolib/hugo_sites.go @@ -559,41 +559,14 @@ func (h *HugoSites) setupTranslations() { } } -func (s *Site) preparePagesForRender(cfg *BuildCfg) { - - pageChan := make(chan *Page) - wg := &sync.WaitGroup{} - - numWorkers := getGoMaxProcs() * 4 - - for i := 0; i < numWorkers; i++ { - wg.Add(1) - go func(pages <-chan *Page, wg *sync.WaitGroup) { - defer wg.Done() - for p := range pages { - p.setContentInit(cfg) - - // In most cases we could delay the content init until rendering time, - // but there could be use cases where the templates would depend - // on state set in the shortcodes (.Page.Scratch.Set), so we - // need to do this early. This will do the needed recursion. - p.initContent() - } - }(pageChan, wg) - } - +func (s *Site) preparePagesForRender(start bool) { for _, p := range s.Pages { - pageChan <- p + p.setContentInit(start) } for _, p := range s.headlessPages { - pageChan <- p + p.setContentInit(start) } - - close(pageChan) - - wg.Wait() - } // Pages returns all pages for all sites. diff --git a/hugolib/hugo_sites_build.go b/hugolib/hugo_sites_build.go index dcff4b3b2..0421c7925 100644 --- a/hugolib/hugo_sites_build.go +++ b/hugolib/hugo_sites_build.go @@ -222,10 +222,21 @@ func (h *HugoSites) assemble(config *BuildCfg) error { func (h *HugoSites) render(config *BuildCfg) error { for _, s := range h.Sites { s.initRenderFormats() - for i, rf := range s.renderFormats { - s.rc = &siteRenderingContext{Format: rf} + } - s.preparePagesForRender(config) + for _, s := range h.Sites { + for i, rf := range s.renderFormats { + for _, s2 := range h.Sites { + // We render site by site, but since the content is lazily rendered + // and a site can "borrow" content from other sites, every site + // needs this set. + s2.rc = &siteRenderingContext{Format: rf} + + isRenderingSite := s == s2 + + s2.preparePagesForRender(isRenderingSite && i == 0) + + } if !config.SkipRender { if err := s.render(config, i); err != nil { diff --git a/hugolib/page.go b/hugolib/page.go index ebaffa57f..9d25cd49a 100644 --- a/hugolib/page.go +++ b/hugolib/page.go @@ -38,6 +38,7 @@ import ( "path" "path/filepath" "regexp" + "runtime" "strings" "sync" "time" @@ -129,9 +130,6 @@ type Page struct { // Params contains configuration defined in the params section of page frontmatter. params map[string]interface{} - // Called when needed to init the content (render shortcodes etc.). - contentInitFn func(p *Page) func() - // Content sections contentv template.HTML summary template.HTML @@ -270,7 +268,14 @@ type Page struct { targetPathDescriptorPrototype *targetPathDescriptor } +func stackTrace() string { + trace := make([]byte, 2000) + runtime.Stack(trace, true) + return string(trace) +} + func (p *Page) initContent() { + p.contentInit.Do(func() { // This careful dance is here to protect against circular loops in shortcode/content // constructs. @@ -284,9 +289,12 @@ func (p *Page) initContent() { p.contentInitMu.Lock() defer p.contentInitMu.Unlock() - if p.contentInitFn != nil { - p.contentInitFn(p)() + err = p.prepareForRender() + if err != nil { + p.s.Log.ERROR.Printf("Failed to prepare page %q for render: %s", p.Path(), err) + return } + if len(p.summary) == 0 { if err = p.setAutoSummary(); err != nil { err = fmt.Errorf("Failed to set user auto summary for page %q: %s", p.pathOrTitle(), err) @@ -297,7 +305,7 @@ func (p *Page) initContent() { select { case <-ctx.Done(): - p.s.Log.WARN.Printf(`WARNING: Timed out creating content for page %q (.Content will be empty). This is most likely a circular shortcode content loop that should be fixed. If this is just a shortcode calling a slow remote service, try to set "timeout=20000" (or higher, value is in milliseconds) in config.toml.`, p.pathOrTitle()) + p.s.Log.WARN.Printf(`WARNING: Timed out creating content for page %q (.Content will be empty). This is most likely a circular shortcode content loop that should be fixed. If this is just a shortcode calling a slow remote service, try to set "timeout=20000" (or higher, value is in milliseconds) in config.toml.\n`, p.pathOrTitle()) case err := <-c: if err != nil { p.s.Log.ERROR.Println(err) @@ -420,14 +428,8 @@ type pageContentInit struct { plainWordsInit sync.Once } -func (p *Page) resetContent(init func(page *Page) func()) { +func (p *Page) resetContent() { p.pageContentInit = &pageContentInit{} - if init == nil { - init = func(page *Page) func() { - return func() {} - } - } - p.contentInitFn = init } // IsNode returns whether this is an item of one of the list types in Hugo, @@ -1165,49 +1167,40 @@ func (p *Page) subResourceTargetPathFactory(base string) string { return path.Join(p.relTargetPathBase, base) } -func (p *Page) setContentInit(cfg *BuildCfg) error { - if !p.shouldRenderTo(p.s.rc.Format) { - // No need to prepare - return nil - } +func (p *Page) setContentInit(start bool) error { - var shortcodeUpdate bool + if start { + // This is a new language. + p.shortcodeState.clearDelta() + } + updated := true if p.shortcodeState != nil { - shortcodeUpdate = p.shortcodeState.updateDelta() + updated = p.shortcodeState.updateDelta() } - resetFunc := func(page *Page) func() { - return func() { - err := page.prepareForRender(cfg) - if err != nil { - p.s.Log.ERROR.Printf("Failed to prepare page %q for render: %s", page.Path(), err) - } - } + if updated { + p.resetContent() } - if shortcodeUpdate || cfg.whatChanged.other { - p.resetContent(resetFunc) - } - - // Handle bundled pages. for _, r := range p.Resources.ByType(pageResourceType) { - shortcodeUpdate = false + p.s.PathSpec.ProcessingStats.Incr(&p.s.PathSpec.ProcessingStats.Pages) bp := r.(*Page) - - if bp.shortcodeState != nil { - shortcodeUpdate = bp.shortcodeState.updateDelta() + if start { + bp.shortcodeState.clearDelta() } - - if shortcodeUpdate || cfg.whatChanged.other { - p.s.PathSpec.ProcessingStats.Incr(&p.s.PathSpec.ProcessingStats.Pages) - bp.resetContent(resetFunc) + if bp.shortcodeState != nil { + updated = bp.shortcodeState.updateDelta() + } + if updated { + bp.resetContent() } } return nil + } -func (p *Page) prepareForRender(cfg *BuildCfg) error { +func (p *Page) prepareForRender() error { s := p.s // If we got this far it means that this is either a new Page pointer @@ -1217,7 +1210,7 @@ func (p *Page) prepareForRender(cfg *BuildCfg) error { // If in watch mode or if we have multiple output formats, // we need to keep the original so we can // potentially repeat this process on rebuild. - needsACopy := p.s.running() || len(p.outputFormats) > 1 + needsACopy := s.running() || len(p.outputFormats) > 1 var workContentCopy []byte if needsACopy { workContentCopy = make([]byte, len(p.workContent)) diff --git a/hugolib/pageSort_test.go b/hugolib/pageSort_test.go index 84711d288..02fa8dcc9 100644 --- a/hugolib/pageSort_test.go +++ b/hugolib/pageSort_test.go @@ -15,7 +15,6 @@ package hugolib import ( "fmt" - "html/template" "path/filepath" "testing" "time" @@ -168,11 +167,16 @@ func setSortVals(dates [4]time.Time, titles [4]string, weights [4]int, pages Pag pages[len(dates)-1-i].linkTitle = pages[i].title + "l" pages[len(dates)-1-i].PublishDate = dates[i] pages[len(dates)-1-i].ExpiryDate = dates[i] - pages[len(dates)-1-i].contentv = template.HTML(titles[i] + "_content") + pages[len(dates)-1-i].workContent = []byte(titles[i] + "_content") } lastLastMod := pages[2].Lastmod pages[2].Lastmod = pages[1].Lastmod pages[1].Lastmod = lastLastMod + + for _, p := range pages { + p.resetContent() + } + } func createSortTestPages(s *Site, num int) Pages { diff --git a/hugolib/page_test.go b/hugolib/page_test.go index bbec1ea42..6b2f29764 100644 --- a/hugolib/page_test.go +++ b/hugolib/page_test.go @@ -525,7 +525,7 @@ func checkPageDate(t *testing.T, page *Page, time time.Time) { } func checkTruncation(t *testing.T, page *Page, shouldBe bool, msg string) { - if page.summary == "" { + if page.Summary() == "" { t.Fatal("page has no summary, can not check truncation") } if page.truncated != shouldBe { @@ -722,8 +722,8 @@ func TestPageWithDelimiterForMarkdownThatCrossesBorder(t *testing.T) { p := s.RegularPages[0] - if p.summary != template.HTML("

The best static site generator.1\n

") { - t.Fatalf("Got summary:\n%q", p.summary) + if p.Summary() != template.HTML("

The best static site generator.1\n

") { + t.Fatalf("Got summary:\n%q", p.Summary()) } if p.content() != template.HTML("

The best static site generator.1\n

\n
\n\n
\n\n
    \n
  1. Many people say so.\n [return]
    2. \n
\n
") { @@ -876,8 +876,8 @@ func TestSummaryWithHTMLTagsOnNextLine(t *testing.T) { assertFunc := func(t *testing.T, ext string, pages Pages) { p := pages[0] - require.Contains(t, p.summary, "Happy new year everyone!") - require.NotContains(t, p.summary, "User interface") + require.Contains(t, p.Summary(), "Happy new year everyone!") + require.NotContains(t, p.Summary(), "User interface") } testAllMarkdownEnginesForPages(t, assertFunc, nil, `--- @@ -1511,8 +1511,8 @@ func TestPageSimpleMethods(t *testing.T) { } { p, _ := s.NewPage("Test") - p.contentv = "

Do Be Do Be Do

" - p.initContent() + p.workContent = []byte("

Do Be Do Be Do

") + p.resetContent() if !this.assertFunc(p) { t.Errorf("[%d] Page method error", i) } diff --git a/hugolib/shortcode.go b/hugolib/shortcode.go index 4792b7f61..8afbfb645 100644 --- a/hugolib/shortcode.go +++ b/hugolib/shortcode.go @@ -366,7 +366,12 @@ func (s *shortcodeHandler) updateDelta() bool { s.contentShortcodes = createShortcodeRenderers(s.shortcodes, s.p.withoutContent()) }) - contentShortcodes := s.contentShortcodesForOutputFormat(s.p.s.rc.Format) + if !s.p.shouldRenderTo(s.p.s.rc.Format) { + // TODO(bep) add test for this re translations + return false + } + of := s.p.s.rc.Format + contentShortcodes := s.contentShortcodesForOutputFormat(of) if s.contentShortcodesDelta == nil || s.contentShortcodesDelta.Len() == 0 { s.contentShortcodesDelta = contentShortcodes @@ -387,13 +392,19 @@ func (s *shortcodeHandler) updateDelta() bool { return delta.Len() > 0 } +func (s *shortcodeHandler) clearDelta() { + if s == nil { + return + } + s.contentShortcodesDelta = newOrderedMap() +} + func (s *shortcodeHandler) contentShortcodesForOutputFormat(f output.Format) *orderedMap { contentShortcodesForOuputFormat := newOrderedMap() lang := s.p.Lang() for _, key := range s.shortcodes.Keys() { shortcodePlaceholder := key.(string) - // shortcodePlaceholder := s.shortcodes.getShortcode(key) key := newScKeyFromLangAndOutputFormat(lang, f, shortcodePlaceholder) renderFn, found := s.contentShortcodes.Get(key) diff --git a/hugolib/shortcode_test.go b/hugolib/shortcode_test.go index b380a5b36..9f86ecb61 100644 --- a/hugolib/shortcode_test.go +++ b/hugolib/shortcode_test.go @@ -22,6 +22,8 @@ import ( "strings" "testing" + "github.com/spf13/viper" + jww "github.com/spf13/jwalterweatherman" "github.com/spf13/afero" @@ -884,7 +886,83 @@ func TestScKey(t *testing.T) { } -func TestPreserveShortcodeOrder(t *testing.T) { +func TestShortcodeGetContent(t *testing.T) { + t.Parallel() + assert := require.New(t) + + contentShortcode := ` +{{- $t := .Get 0 -}} +{{- $p := .Get 1 -}} +{{- $k := .Get 2 -}} +{{- $page := $.Page.Site.GetPage "page" $p -}} +{{ if $page }} +{{- if eq $t "bundle" -}} +{{- .Scratch.Set "p" ($page.Resources.GetMatch (printf "%s*" $k)) -}} +{{- else -}} +{{- $.Scratch.Set "p" $page -}} +{{- end -}}P1:{{ .Page.Content }}|P2:{{ $p := ($.Scratch.Get "p") }}{{ $p.Title }}/{{ $p.Content }}| +{{- else -}} +{{- errorf "Page %s is nil" $p -}} +{{- end -}} +` + + var templates []string + var content []string + + contentWithShortcodeTemplate := `--- +title: doc%s +weight: %d +--- +Logo:{{< c "bundle" "b1" "logo.png" >}}:P1: {{< c "page" "section1/p1" "" >}}:BP1:{{< c "bundle" "b1" "bp1" >}}` + + simpleContentTemplate := `--- +title: doc%s +weight: %d +--- +C-%s` + + v := viper.New() + + v.Set("timeout", 500) + + templates = append(templates, []string{"shortcodes/c.html", contentShortcode}...) + templates = append(templates, []string{"_default/single.html", "Single Content: {{ .Content }}"}...) + templates = append(templates, []string{"_default/list.html", "List Content: {{ .Content }}"}...) + + content = append(content, []string{"b1/index.md", fmt.Sprintf(contentWithShortcodeTemplate, "b1", 1)}...) + content = append(content, []string{"b1/logo.png", "PNG logo"}...) + content = append(content, []string{"b1/bp1.md", fmt.Sprintf(simpleContentTemplate, "bp1", 1, "bp1")}...) + + content = append(content, []string{"section1/_index.md", fmt.Sprintf(contentWithShortcodeTemplate, "s1", 2)}...) + content = append(content, []string{"section1/p1.md", fmt.Sprintf(simpleContentTemplate, "s1p1", 2, "s1p1")}...) + + content = append(content, []string{"section2/_index.md", fmt.Sprintf(simpleContentTemplate, "b1", 1, "b1")}...) + content = append(content, []string{"section2/s2p1.md", fmt.Sprintf(contentWithShortcodeTemplate, "bp1", 1)}...) + + builder := newTestSitesBuilder(t).WithDefaultMultiSiteConfig() + + builder.WithViper(v).WithContent(content...).WithTemplates(templates...).CreateSites().Build(BuildCfg{}) + s := builder.H.Sites[0] + assert.Equal(3, len(s.RegularPages)) + + builder.AssertFileContent("public/section1/index.html", + "List Content:

Logo:P1:|P2:logo.png/PNG logo|:P1: P1:|P2:docs1p1/

C-s1p1

\n|", + "BP1:P1:|P2:docbp1/

C-bp1

", + ) + + builder.AssertFileContent("public/b1/index.html", + "Single Content:

Logo:P1:|P2:logo.png/PNG logo|:P1: P1:|P2:docs1p1/

C-s1p1

\n|", + "P2:docbp1/

C-bp1

", + ) + + builder.AssertFileContent("public/section2/s2p1/index.html", + "Single Content:

Logo:P1:|P2:logo.png/PNG logo|:P1: P1:|P2:docs1p1/

C-s1p1

\n|", + "P2:docbp1/

C-bp1

", + ) + +} + +func TestShortcodePreserveOrder(t *testing.T) { t.Parallel() assert := require.New(t) @@ -897,28 +975,30 @@ weight: %d {{< s1 >}}{{< s2 >}}{{< s3 >}}{{< s4 >}}{{< s5 >}} {{< nested >}} -{{< ordinal >}} -{{< ordinal >}} -{{< ordinal >}} +{{< ordinal >}} {{< scratch >}} +{{< ordinal >}} {{< scratch >}} +{{< ordinal >}} {{< scratch >}} {{< /nested >}} - ` - ordinalShortcodeTemplate := `ordinal: {{ .Ordinal }}` + ordinalShortcodeTemplate := `ordinal: {{ .Ordinal }}{{ .Page.Scratch.Set "ordinal" .Ordinal }}` nestedShortcode := `outer ordinal: {{ .Ordinal }} inner: {{ .Inner }}` - - shortCodeTemplate := `v%d: {{ .Ordinal }}|` + scratchGetShortcode := `scratch ordinal: {{ .Ordinal }} scratch get ordinal: {{ .Page.Scratch.Get "ordinal" }}` + shortcodeTemplate := `v%d: {{ .Ordinal }} sgo: {{ .Page.Scratch.Get "o2" }}{{ .Page.Scratch.Set "o2" .Ordinal }}|` var shortcodes []string var content []string shortcodes = append(shortcodes, []string{"shortcodes/nested.html", nestedShortcode}...) shortcodes = append(shortcodes, []string{"shortcodes/ordinal.html", ordinalShortcodeTemplate}...) + shortcodes = append(shortcodes, []string{"shortcodes/scratch.html", scratchGetShortcode}...) for i := 1; i <= 5; i++ { - shortcodes = append(shortcodes, []string{fmt.Sprintf("shortcodes/s%d.html", i), fmt.Sprintf(shortCodeTemplate, i)}...) + sc := fmt.Sprintf(shortcodeTemplate, i) + sc = strings.Replace(sc, "%%", "%", -1) + shortcodes = append(shortcodes, []string{fmt.Sprintf("shortcodes/s%d.html", i), sc}...) } for i := 1; i <= 3; i++ { @@ -932,18 +1012,10 @@ weight: %d s := builder.H.Sites[0] assert.Equal(3, len(s.RegularPages)) - p1 := s.RegularPages[0] - - if !strings.Contains(string(p1.content()), `v1: 0|v2: 1|v3: 2|v4: 3|v5: 4|`) { - t.Fatal(p1.content()) - } - - // Check nested behaviour - if !strings.Contains(string(p1.content()), `outer ordinal: 5 inner: -ordinal: 0 -ordinal: 1 -ordinal: 2`) { - t.Fatal(p1.content()) - } + builder.AssertFileContent("public/en/p1/index.html", `v1: 0 sgo: |v2: 1 sgo: 0|v3: 2 sgo: 1|v4: 3 sgo: 2|v5: 4 sgo: 3`) + builder.AssertFileContent("public/en/p1/index.html", `outer ordinal: 5 inner: +ordinal: 0 scratch ordinal: 1 scratch get ordinal: 0 +ordinal: 2 scratch ordinal: 3 scratch get ordinal: 2 +ordinal: 4 scratch ordinal: 5 scratch get ordinal: 4`) } diff --git a/hugolib/site.go b/hugolib/site.go index 83b575f36..2a55e4330 100644 --- a/hugolib/site.go +++ b/hugolib/site.go @@ -180,6 +180,7 @@ func (s *Site) reset() *Site { titleFunc: s.titleFunc, relatedDocsHandler: newSearchIndexHandler(s.relatedDocsHandler.cfg), outputFormats: s.outputFormats, + rc: s.rc, outputFormatsConfig: s.outputFormatsConfig, frontmatterHandler: s.frontmatterHandler, mediaTypesConfig: s.mediaTypesConfig, @@ -266,6 +267,7 @@ func newSite(cfg deps.DepsCfg) (*Site, error) { titleFunc: titleFunc, relatedDocsHandler: newSearchIndexHandler(relatedContentConfig), outputFormats: outputFormats, + rc: &siteRenderingContext{output.HTMLFormat}, outputFormatsConfig: siteOutputFormatsConfig, mediaTypesConfig: siteMediaTypesConfig, frontmatterHandler: frontMatterHandler, @@ -546,7 +548,7 @@ func (s *SiteInfo) RelRef(ref string, page *Page, options ...string) (string, er } func (s *Site) running() bool { - return s.owner.running + return s.owner != nil && s.owner.running } func init() {