From 6889b5a8093094e1ff674b7e6a987e5c7e2821ed Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 4 Mar 2019 15:52:34 -0500 Subject: [PATCH] Asciidoctor: Make Edit Me links know about repos The "Edit Me" link generation in Asciidoctor was basically a copy of the asciidoc implementation which was simple because it *had* to be simple. The asciidoctor implementation had trouble dealing with books built from multiple repositories because the asciidoc implementation does. The asciidoc implementation works around it by allowing files to set their `edit_url` whenever they include a book from a different repo. We had a bug the asciidoctor implementation that led to cross repo links not working that was unrelated, but it led me to notice how silly this whole thing was. This change replaces way that asciidoctor finds the edit url. We now pass in an attribute with the root of each repo and the edit me url for that root. When we want to generate the a link we pick the first repo that contains the path of the asciidoctor file and use it's associated url. This is nice because it dosen't need anything like `repo_root` and it can automatically handle the edit me links, allowing us to remove the overridden links when we shift books to asciidoctor. --- build_docs.pl | 43 +++++-- lib/ES/Book.pm | 7 +- lib/ES/Source.pm | 21 ++-- lib/ES/Toc.pm | 7 +- lib/ES/Util.pm | 29 +++-- .../asciidoctor/lib/edit_me/extension.rb | 50 ++++++-- resources/asciidoctor/spec/edit_me_spec.rb | 115 ++++++++++-------- 7 files changed, 176 insertions(+), 96 deletions(-) diff --git a/build_docs.pl b/build_docs.pl index f1536a6774df2..304097e727bba 100755 --- a/build_docs.pl +++ b/build_docs.pl @@ -137,16 +137,43 @@ sub _guess_opts_from_file { #=================================== my $index = shift; + my %edit_urls = (); + my $doc_toplevel = _find_toplevel($index->parent); + if ( $doc_toplevel ) { + $Opts->{root_dir} = $doc_toplevel; + my $edit_url = _guess_edit_url($doc_toplevel); + @edit_urls{ $doc_toplevel } = $edit_url if $edit_url; + } else { + $Opts->{root_dir} = $index->parent; + } + for my $resource ( @{ $Opts->{resource} } ) { + my $resource_toplevel = _find_toplevel($resource); + next unless $resource_toplevel; + + my $resource_edit_url = _guess_edit_url($resource_toplevel); + @edit_urls{ $resource_toplevel } = $resource_edit_url if $resource_edit_url; + } + $Opts->{edit_urls} = { %edit_urls }; +} + +#=================================== +sub _find_toplevel { +#=================================== + my $docpath = shift; + my $original_pwd = Cwd::cwd(); - chdir $index->parent; + chdir $docpath; my $toplevel = eval { run qw(git rev-parse --show-toplevel) }; chdir $original_pwd; - unless ( $toplevel ) { - say "Couldn't find edit url because the document doesn't look like it is in git"; - $Opts->{root_dir} = $index->parent; - return; - } - $Opts->{root_dir} = $toplevel; + say "Couldn't find repo toplevel for $docpath" unless $toplevel; + return $toplevel; +} + +#=================================== +sub _guess_edit_url { +#=================================== + my $toplevel = shift; + local $ENV{GIT_DIR} = dir($toplevel)->subdir('.git'); my $remotes = eval { run qw(git remote -v) } || ''; if ($remotes !~ m|\s+(\S+[/:]elastic/\S+)|) { @@ -156,7 +183,7 @@ sub _guess_opts_from_file { } my $remote = $1; my $branch = eval {run qw(git rev-parse --abbrev-ref HEAD) } || 'master'; - $Opts->{edit_url} = ES::Repo::edit_url_for_url_and_branch($remote, $branch); + return ES::Repo::edit_url_for_url_and_branch($remote, $branch); } #=================================== diff --git a/lib/ES/Book.pm b/lib/ES/Book.pm index 4c7a35bce2b1d..9f7871decfe02 100644 --- a/lib/ES/Book.pm +++ b/lib/ES/Book.pm @@ -227,7 +227,6 @@ sub _build_book { my $index = $self->index; my $section_title = $self->section_title($branch); my $subject = $self->subject; - my $edit_url = $self->source->edit_url($branch); my $lang = $self->lang; return @@ -236,7 +235,7 @@ sub _build_book { && !$template->md5_changed($branch_dir) && !$source->has_changed( $self->title, $branch, $self->asciidoctor ); - my ( $checkout, $first_path ) = $source->prepare($branch); + my ( $checkout, $edit_urls, $first_path ) = $source->prepare($branch); $pm->start($branch) and return; say " - Branch: $branch - Building..."; @@ -249,7 +248,7 @@ sub _build_book { $branch_dir, version => $branch, lang => $lang, - edit_url => $edit_url, + edit_urls => $edit_urls, root_dir => $first_path, private => $self->private, noindex => $self->noindex, @@ -269,7 +268,7 @@ sub _build_book { $branch_dir, version => $branch, lang => $lang, - edit_url => $edit_url, + edit_urls => $edit_urls, root_dir => $first_path, private => $self->private, noindex => $self->noindex, diff --git a/lib/ES/Source.pm b/lib/ES/Source.pm index 77e378835d6e1..c8b2b1c318527 100644 --- a/lib/ES/Source.pm +++ b/lib/ES/Source.pm @@ -36,14 +36,6 @@ sub first { return shift->_sources->[0]; } -#=================================== -sub edit_url { -#=================================== - my $self = shift; - my $branch = shift; - return $self->first->{repo}->edit_url($branch); -} - #=================================== sub has_changed { #=================================== @@ -92,19 +84,22 @@ sub prepare { my $self = shift; my $branch = shift; - my %entries; - my $dest = Path::Class::tempdir( DIR => $self->temp_dir ); + my $checkout = Path::Class::tempdir( DIR => $self->temp_dir ); + my %edit_urls = (); + my $first_path = 0; # need to handle repo name here, not in Repo for my $source ( $self->_sources_for_branch($branch) ) { my $repo = $source->{repo}; my $prefix = $source->{prefix}; my $path = $source->{path}; + my $source_checkout = $checkout->subdir($prefix); - $repo->extract( $branch, $path, $dest->subdir($prefix) ); - + $repo->extract( $branch, $path, $source_checkout ); + $edit_urls{ $source_checkout->absolute } = $repo->edit_url($branch); + $first_path = $source_checkout unless $first_path; } - return ( $dest, $dest->subdir( $self->first->{prefix} ) ); + return ( $checkout, \%edit_urls, $first_path ); } #=================================== diff --git a/lib/ES/Toc.pm b/lib/ES/Toc.pm index 76d940a121e88..1867ab0483593 100644 --- a/lib/ES/Toc.pm +++ b/lib/ES/Toc.pm @@ -36,7 +36,12 @@ sub write { my $adoc_file = $dir->file('index.asciidoc'); $adoc_file->spew( iomode => '>:utf8', $adoc ); - build_single( $adoc_file, $dir, type => 'article', lang => $self->lang ); + build_single( $adoc_file, $dir, + type => 'article', + lang => $self->lang, + root_dir => '', + edit_urls => {'' => ''}, + ); $adoc_file->remove; } diff --git a/lib/ES/Util.pm b/lib/ES/Util.pm index 426bd4ef5a3ed..b60c9b750230c 100644 --- a/lib/ES/Util.pm +++ b/lib/ES/Util.pm @@ -37,8 +37,8 @@ sub build_chunked { my $multi = $opts{multi} || 0; my $lenient = $opts{lenient} || ''; my $lang = $opts{lang} || 'en'; - my $edit_url = $opts{edit_url} || ''; - my $root_dir = $opts{root_dir} || ''; + my $edit_urls = $opts{edit_urls}; + my $root_dir = $opts{root_dir}; my $section = $opts{section_title} || ''; my $subject = $opts{subject} || ''; my $private = $opts{private} || ''; @@ -90,11 +90,11 @@ sub build_chunked { '-d' => 'book', '-a' => 'showcomments=1', '-a' => "lang=$lang", - '-a' => 'repo_root=' . $root_dir, # Use ` to delimit monospaced literals because our docs # expect that '-a' => 'compat-mode=legacy', - $private ? () : ( '-a' => "edit_url=$edit_url" ), + $private ? () : ( '-a' => "edit_urls=" . + edit_urls_for_asciidoctor($edit_urls) ), # Disable warning on missing attributes because we have # missing attributes! # '-a' => 'attribute-missing=warn', @@ -121,6 +121,7 @@ sub build_chunked { } or do { $output = $@; $died = 1; }; } else { + my $edit_url = $edit_urls->{$root_dir}; eval { $output = run( 'a2x', '-v', #'--keep', @@ -172,8 +173,8 @@ sub build_single { my $version = $opts{version} || ''; my $multi = $opts{multi} || 0; my $lang = $opts{lang} || 'en'; - my $edit_url = $opts{edit_url} || ''; - my $root_dir = $opts{root_dir} || ''; + my $edit_urls = $opts{edit_urls}; + my $root_dir = $opts{root_dir}; my $section = $opts{section_title} || ''; my $subject = $opts{subject} || ''; my $private = $opts{private} || ''; @@ -222,8 +223,8 @@ sub build_single { '-d' => $type, '-a' => 'showcomments=1', '-a' => "lang=$lang", - '-a' => 'repo_root=' . $root_dir, - $private ? () : ( '-a' => "edit_url=$edit_url" ), + $private ? () : ( '-a' => "edit_urls=" . + edit_urls_for_asciidoctor($edit_urls) ), '-a' => 'asciidoc-dir=' . $asciidoc_dir, '-a' => 'resources=' . join(',', @$resources), '-a' => 'copy-callout-images=png', @@ -250,6 +251,7 @@ sub build_single { } or do { $output = $@; $died = 1; }; } else { + my $edit_url = $edit_urls->{$root_dir}; eval { $output = run( 'a2x', '-v', @@ -444,6 +446,17 @@ sub rawxsltopts { return @opts; } +#=================================== +sub edit_urls_for_asciidoctor { +#=================================== + my $edit_urls = shift; + + # We'd be better off using a csv library for this but we don't want to add + # more dependencies to the pl until we go docker-only. + return join("\n", map { "$_,$edit_urls->{$_}" } keys %{$edit_urls}); +} + + #=================================== sub write_html_redirect { #=================================== diff --git a/resources/asciidoctor/lib/edit_me/extension.rb b/resources/asciidoctor/lib/edit_me/extension.rb index 5f8763730c110..6a27917c425a3 100644 --- a/resources/asciidoctor/lib/edit_me/extension.rb +++ b/resources/asciidoctor/lib/edit_me/extension.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'pathname' +require 'csv' require_relative '../scaffold.rb' ## @@ -12,32 +12,56 @@ class EditMe < TreeProcessorScaffold def process(document) logger.error("sourcemap is required") unless document.sourcemap - super if document.attributes['edit_url'] + edit_urls_string = document.attributes['edit_urls'] + return unless edit_urls_string + + edit_urls = [] + CSV.parse edit_urls_string do |toplevel, url| + unless toplevel + logger.error message_with_context "invalid edit_urls, no toplevel" + next + end + unless url + logger.error message_with_context "invalid edit_urls, no url" + next + end + url = url[0..-2] if url.end_with? '/' + edit_urls << { toplevel: toplevel, url: url } + end + document.attributes['edit_urls'] = edit_urls + super end def process_block(block) return unless %i[preamble section floating_title].include? block.context def block.title - path = source_path - url = @document.attributes['edit_url'] - url += '/' unless url.end_with?('/') - repo_root = @document.attributes['repo_root'] - if repo_root - repo_root = Pathname.new repo_root - base_dir = Pathname.new @document.base_dir - url += "#{base_dir.relative_path_from(repo_root)}/" unless repo_root == base_dir + # || '' allows us to not blow up when translating strings that + # aren't associated with any particular file. '' is asciidoctor's + # standard name for such strings. + path = source_path || '' + + edit_urls = @document.attributes['edit_urls'] + edit_url = edit_urls.find { |e| path.start_with? e[:toplevel] } + unless edit_url + logger.warn message_with_context "couldn't find edit url for #{path}", :source_location => source_location + return super end - url += path + url = edit_url[:url] + url += path[edit_url[:toplevel].length..-1] "#{super}Edit me" end if block.context == :preamble def block.source_path - document.source_location.path + # source_location.path doesn't work for relative includes outside of + # the base_dir which we use when we build books from many repos. + document.source_location.file end else def block.source_path - source_location.path + # source_location.path doesn't work for relative includes outside of + # the base_dir which we use when we build books from many repos. + source_location.file end end end diff --git a/resources/asciidoctor/spec/edit_me_spec.rb b/resources/asciidoctor/spec/edit_me_spec.rb index 422047a502c00..2f617614c3170 100644 --- a/resources/asciidoctor/spec/edit_me_spec.rb +++ b/resources/asciidoctor/spec/edit_me_spec.rb @@ -1,5 +1,6 @@ # frozen_string_literal: true +require 'pathname' require 'edit_me/extension' RSpec.describe EditMe do @@ -13,9 +14,33 @@ Asciidoctor::Extensions.unregister_all end + spec_dir = File.dirname(__FILE__) + + it "has a nice error message if you are missing the edit url" do + attributes = { + 'edit_urls' => '', + } + warnings = <<~WARNINGS + ERROR: invalid edit_urls, no url + WARN: couldn't find edit url for + WARNINGS + convert 'Words.', attributes, eq(warnings.strip) + end + + it "has a nice error message if you are missing the toplevel" do + attributes = { + 'edit_urls' => ',http://example.com', + } + warnings = <<~WARNINGS + ERROR: invalid edit_urls, no toplevel + WARN: couldn't find edit url for + WARNINGS + convert 'Words.', attributes, eq(warnings.strip) + end + it "adds a link to the preface" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => ',www.example.com/docs', } input = <<~ASCIIDOC :preface-title: Preface @@ -23,7 +48,7 @@ ASCIIDOC expected = <<~DOCBOOK - Preface<ulink role="edit_me" url="www.example.com/docs/<stdin>">Edit me</ulink> + Preface<ulink role="edit_me" url="www.example.com/docs">Edit me</ulink> Words. DOCBOOK @@ -46,7 +71,7 @@ it "adds a link to each chapter title" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => "#{spec_dir},www.example.com/docs", } input = <<~ASCIIDOC include::resources/edit_me/chapter1.adoc[] @@ -66,44 +91,6 @@ expect(convert input, attributes).to eq(expected.strip) end - it "respects the repo_root attribute" do - attributes = { - 'edit_url' => 'www.example.com/docs/', - 'repo_root' => File.dirname(File.dirname(__FILE__)), - } - input = <<~ASCIIDOC - include::resources/edit_me/chapter1.adoc[] - - include::resources/edit_me/chapter2.adoc[] - ASCIIDOC - expect(convert input, attributes).to match(%r{ - ^.+ - url="www\.example\.com/docs/spec/resources/edit_me/chapter1\.adoc" - .+ - url="www\.example\.com/docs/spec/resources/edit_me/chapter2\.adoc" - .+$ - }xm) - end - - it "doesn't add extra path segments if repo_root is the base_dir" do - attributes = { - 'edit_url' => 'www.example.com/docs/', - 'repo_root' => File.dirname(__FILE__), - } - input = <<~ASCIIDOC - include::resources/edit_me/chapter1.adoc[] - - include::resources/edit_me/chapter2.adoc[] - ASCIIDOC - expect(convert input, attributes).to match(%r{ - ^.+ - url="www\.example\.com/docs/resources/edit_me/chapter1\.adoc" - .+ - url="www\.example\.com/docs/resources/edit_me/chapter2\.adoc" - .+$ - }xm) - end - it "does not add a link to each chapter title if edit_link is not set" do input = <<~ASCIIDOC include::resources/edit_me/chapter1.adoc[] @@ -125,7 +112,7 @@ it "adds a link to each section title" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => "#{spec_dir},www.example.com/docs/", } input = <<~ASCIIDOC include::resources/edit_me/section1.adoc[] @@ -166,7 +153,7 @@ it "adds a link to each appendix title" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => "#{spec_dir},www.example.com/docs", } input = <<~ASCIIDOC include::resources/edit_me/appendix1.adoc[] @@ -207,7 +194,7 @@ it "adds a link to each glossary title" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => "#{spec_dir},www.example.com/docs", } input = <<~ASCIIDOC include::resources/edit_me/glossary1.adoc[] @@ -248,7 +235,7 @@ it "adds a link to each bibliography title" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => "#{spec_dir},www.example.com/docs", } input = <<~ASCIIDOC include::resources/edit_me/bibliography1.adoc[] @@ -289,7 +276,7 @@ it "adds a link to each dedication title" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => "#{spec_dir},www.example.com/docs", } input = <<~ASCIIDOC include::resources/edit_me/dedication1.adoc[] @@ -330,7 +317,7 @@ it "adds a link to each colophon title" do attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => "#{spec_dir},www.example.com/docs", } input = <<~ASCIIDOC include::resources/edit_me/colophon1.adoc[] @@ -370,8 +357,12 @@ end it "adds a link to each floating title" do + edit_urls = <<~CSV + ,www.example.com/stdin + #{spec_dir},www.example.com/docs + CSV attributes = { - 'edit_url' => 'www.example.com/docs/', + 'edit_urls' => edit_urls, } input = <<~ASCIIDOC == Chapter @@ -382,7 +373,7 @@ ASCIIDOC expected = <<~DOCBOOK - Chapter<ulink role="edit_me" url="www.example.com/docs/<stdin>">Edit me</ulink> + Chapter<ulink role="edit_me" url="www.example.com/stdin">Edit me</ulink> Float 1Edit me Words. Float 2Edit me @@ -411,4 +402,30 @@ DOCBOOK expect(convert input).to eq(expected.strip) end + + it "can handle multiple edit urls" do + edit_urls = <<~CSV + #{spec_dir}/resources/edit_me/chapter1.adoc,www.example.com/1 + #{spec_dir}/resources/edit_me/chapter2.adoc,www.example.com/2 + CSV + attributes = { + 'edit_urls' => edit_urls, + } + input = <<~ASCIIDOC + include::resources/edit_me/chapter1.adoc[] + + include::resources/edit_me/chapter2.adoc[] + ASCIIDOC + expected = <<~DOCBOOK + + Chapter 1<ulink role="edit_me" url="www.example.com/1">Edit me</ulink> + Words. + + + Chapter 2<ulink role="edit_me" url="www.example.com/2">Edit me</ulink> + Words. + + DOCBOOK + expect(convert input, attributes).to eq(expected.strip) + end end