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

Allow building all docs with a local repo #659

Merged
merged 24 commits into from
Mar 11, 2019
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions build_docs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ from __future__ import print_function

import logging
from os import environ, getgid, getuid
from os.path import basename, dirname, exists, isdir, join, realpath
from os.path import basename, dirname, exists, expanduser, isdir
from os.path import join, realpath
import re
import subprocess
from sys import platform, version_info
Expand Down Expand Up @@ -117,8 +118,7 @@ def run_build_docs(args):
'-v',
'%s:%s:ro,cached' % (repo_root, repo_mount)
])
build_docs_args.append(
repo_mount + path.replace(repo_root, ''))
return repo_mount + path.replace(repo_root, '')

open_browser = False
args = Args(args)
Expand Down Expand Up @@ -164,7 +164,8 @@ def run_build_docs(args):
doc_file = realpath(args.next_arg_or_err())
if not exists(doc_file):
raise ArgError("Can't find --doc %s" % doc_file)
mount_docs_repo_and_dockerify_path(dirname(doc_file), doc_file)
build_docs_args.append(mount_docs_repo_and_dockerify_path(
dirname(doc_file), doc_file))
saw_doc = True
elif arg == '--open':
docker_args.extend(['--publish', '8000:8000/tcp'])
Expand Down Expand Up @@ -208,7 +209,19 @@ def run_build_docs(args):
resource_dir = realpath(args.next_arg_or_err())
if not isdir(resource_dir):
raise ArgError("Can't find --resource %s" % resource_dir)
mount_docs_repo_and_dockerify_path(resource_dir, resource_dir)
build_docs_args.append(mount_docs_repo_and_dockerify_path(
resource_dir, resource_dir))
elif arg == '--sub_dir':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you add features, all the manual parsing of argv is going to cost you more and more. argparse isn't great, but is there any value in considering it over full DIY?

(Normally, I would suggest Click, but I know you can't do packages.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've considered it on and off for a while. I think the value that DIY gets me is being able to pass through arguments that I don't know about. I only rewrite the arguments that have anything to do with paths. The cost of DIY is that I need this everywhere. At this point I think it is net simpler, but I'd be receptive to any argument to the contrary. Like, I wonder if it'd be helpful to force me to list all of the supported arguments in the python file because we plan to drop the perl file one day.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I've considered it"

Cool. I trust your judgement.

sub = args.next_arg_or_err()
m = re.match('(?P<repo>[^:]+):(?P<branch>[^:]+):(?P<dir>.+)', sub)
if not m:
raise ArgError("Invalid --sub_dir %s" % sub)
sub_dir = realpath(expanduser(m.group('dir')))
if not exists(sub_dir):
raise ArgError("Can't find --sub_dir %s" % sub_dir)
mounted_path = mount_docs_repo_and_dockerify_path(sub_dir, sub_dir)
build_docs_args.append("%s:%s:%s" % (
m.group('repo'), m.group('branch'), mounted_path))
arg = args.next_arg()

if saw_doc and not saw_out:
Expand Down
23 changes: 19 additions & 4 deletions build_docs.pl
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ BEGIN

GetOptions(
$Opts, #
'all', 'push', 'target_repo=s', 'reference=s', 'rebuild', 'no_fetch', #
'all', 'push', 'target_repo=s', 'reference=s', 'rebuild', 'keep_hash', 'sub_dir=s@',
'single', 'pdf', 'doc=s', 'out=s', 'toc', 'chunk=i',
'open', 'skiplinkcheck', 'linkcheckonly', 'staging', 'procs=i', 'user=s', 'lang=s',
'lenient', 'verbose', 'reload_template', 'resource=s@', 'asciidoctor', 'in_standard_docker',
Expand Down Expand Up @@ -453,6 +453,7 @@ sub init_repos {
user => $Opts->{user},
url => $Opts->{target_repo},
reference => $reference_dir,
keep_hash => 0,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't keep_hash on the target repo because the target_repo contains the list of hashes to keep!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's worth a review comment, is it worth a code comment?

# intentionally not passing the tracker because we don't want to use it
);
delete $child_dirs{ $target_repo->git_dir->absolute };
Expand Down Expand Up @@ -487,6 +488,7 @@ sub init_repos {
user => $Opts->{user},
url => $url,
reference => $reference_dir,
keep_hash => $Opts->{keep_hash},
);
delete $child_dirs{ $repo->git_dir->absolute };

Expand All @@ -496,7 +498,7 @@ sub init_repos {
else {
$pm->start($name) and next;
eval {
$repo->update_from_remote() unless $Opts->{no_fetch};
$repo->update_from_remote();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I drop this feature because --keep_hash is better in most cases.

1;
} or do {
# If creds are invalid, explicitly reject them to try to clear the cache
Expand All @@ -511,6 +513,16 @@ sub init_repos {
}
$pm->wait_all_children;

# Parse the --sub_dir options and attach the to the repo
my %sub_dirs = ();
foreach (@{ $Opts->{sub_dir} }) {
die "invalid --sub_dir $_"
unless /(?<repo>[^:]+):(?<branch>[^:]+):(?<dir>.+)/;
my $dir = dir($+{dir})->absolute;
die "--sub_dir $dir doesn't exist" unless -e $dir;
ES::Repo->get_repo($+{repo})->add_sub_dir($+{branch}, $dir);
}

for ( keys %child_dirs ) {
my $dir = dir($_);
next unless -d $dir;
Expand Down Expand Up @@ -671,7 +683,8 @@ sub check_args {
die('--user not compatible with --doc') if $Opts->{user};
die('--reference not compatible with --doc') if $Opts->{reference};
die('--rebuild not compatible with --doc') if $Opts->{rebuild};
die('--no_fetch not compatible with --doc') if $Opts->{no_fetch};
die('--keep_hash not compatible with --doc') if $Opts->{keep_hash};
die('--sub_dir not compatible with --doc') if $Opts->{sub_dir};
die('--skiplinkcheck not compatible with --doc') if $Opts->{skiplinkcheck};
die('--linkcheckonly not compatible with --doc') if $Opts->{linkcheckonly};
} else {
Expand Down Expand Up @@ -809,7 +822,9 @@ sub usage {
--skiplinkcheck Omit the step that checks for broken links
--linkcheckonly Skips the documentation builds. Checks links only.
--rebuild Rebuild all branches of every book regardless of what has changed
--no_fetch Skip fetching updates from source repos
--keep_hash Build docs from the same commit hash as last time
--sub_dir Use a directory as a branch of some repo
(eg --sub_dir elasticsearch:master:~/Code/elasticsearch)
ninaspitfire marked this conversation as resolved.
Show resolved Hide resolved

General Opts:
--staging Use the template from the staging website
Expand Down
159 changes: 140 additions & 19 deletions integtest/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ check: \
experimental_expected_files experimental_same_files \
missing_include_fails_asciidoc missing_include_fails_asciidoctor \
readme_expected_files readme_same_files \
small_all_expected_files
simple_all_expected_files \
keep_hash_expected_files \
sub_dir_expected_files \
keep_hash_and_sub_dir_expected_files

.PHONY: style
style: html_diff
Expand Down Expand Up @@ -117,37 +120,155 @@ missing_include_fails_asciidoctor: missing_include.asciidoc
/tmp/%_asciidoctor:
$(BD) --asciidoctor --doc $*.asciidoc

.PHONY: small_all_expected_files
small_all_expected_files: /tmp/small_all
[ -s $^/html/branches.yaml ]
grep '<a class="ulink" href="test/current/index.html" target="_top">Test book</a>' $^/html/index.html > /dev/null
grep '<meta http-equiv="refresh" content="0; url=current/index.html">' $^/html/test/index.html > /dev/null
[ -s $^/html/test/current/index.html ]
.PHONY: simple_all_expected_files
simple_all_expected_files:
# Test the simplest possible `--all` invocation
$(call BUILD_MINIMAL_ALL,/tmp/simple_all)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to these call invocations because they don't bump into make's broken up-to-date checking for directories.

$(call MINIMAL_ALL_EXPECTED_FILES,/tmp/simple_all)

.PRECIOUS: /tmp/small_all
/tmp/small_all:
# Builds "--all" documentation specified by by the "small_conf.yaml" file.
.PHONY: keep_hash_expected_files
keep_hash_expected_files:
# Test that `--all --keep_hash` doesn't pull new updates
$(call BUILD_MINIMAL_ALL,/tmp/keep_hash)

# First build a repository to use as the source.
# REPLACE the minimal documentation in the source repo
cp ../README.asciidoc /tmp/source/index.asciidoc
cd /tmp/source && \
Copy link
Contributor

@ninaspitfire ninaspitfire Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this ever be run in parallel with itself? Might need a proper tmpdir.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be difficult to get it to run in parallel with itself but I may as well make a proper tmpdir. You are right about that!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess Docker kinda takes care of that, doesn't it? I may have been thinking too old-school.

git add . && \
git commit -m 'README'

# Rebuild the docs with --keep_hash which should ignore the replacement
/docs_build/build_docs.pl --in_standard_docker --all --push \
--target_repo /tmp/keep_hash.git \
--conf small_conf.yaml \
--keep_hash | tee /tmp/out
$(call GREP,'No changes to push',/tmp/out)

# We expact the same files as the minimal because we the changes that we
# make shouldn't be included
$(call MINIMAL_ALL_EXPECTED_FILES,/tmp/keep_hash)

.PHONY: sub_dir_expected_file
sub_dir_expected_files:
# Test that `--all --sub_dir` substitutes a directory for a branch of
# a repo.

# We still need to build the source repo because the script wants to fetch
# it just in case we need another branch.
rm -rf /tmp/source
git init /tmp/source
cp minimal.asciidoc /tmp/source/
cd /tmp/source && \
cd /tmp/source && git commit --allow-empty -m "empty"

# Setup the directory we'd like to substitute
rm -rf /tmp/to_sub
mkdir /tmp/to_sub
cp minimal.asciidoc /tmp/to_sub/index.asciidoc

rm -rf /tmp/sub_dir.git
git init --bare /tmp/sub_dir.git
/docs_build/build_docs.pl --in_standard_docker --all --push \
--target_repo /tmp/sub_dir.git \
--conf small_conf.yaml \
--sub_dir source:master:/tmp/to_sub

$(call MINIMAL_ALL_EXPECTED_FILES,/tmp/sub_dir)

keep_hash_and_sub_dir_expected_files:
# Test that `--all --keep_hash --sub_dir` keeps hashes the same for repos
# not specified by --sub_dir but forces rebuilding all books that include
# --sub_dir.

$(call INIT_REPO_WITH_FILE,/tmp/source1,includes_source2.asciidoc,docs/index.asciidoc)
$(call INIT_REPO_WITH_FILE,/tmp/source2,included.asciidoc,index.asciidoc)
rm -rf /tmp/keep_hash_and_sub_dir.git
git init --bare /tmp/keep_hash_and_sub_dir.git

/docs_build/build_docs.pl --in_standard_docker --all --push \
--target_repo /tmp/keep_hash_and_sub_dir.git \
--conf two_repos_conf.yaml

# Move a "bad" file into source2 so we can be sure we're not picking it up
cp ../README.asciidoc /tmp/source2/index.asciidoc
cd /tmp/source2 && \
git add . && \
git commit -m 'minimal'
git commit -m 'README'

/docs_build/build_docs.pl --in_standard_docker --all --push \
--target_repo /tmp/keep_hash_and_sub_dir.git \
--conf two_repos_conf.yaml \
--keep_hash | tee /tmp/out
$(call GREP,'No changes to push',/tmp/out)

# Setup the directory we'd like to substitute
rm -rf /tmp/to_sub
mkdir -p /tmp/to_sub/docs
cp includes_source2.asciidoc /tmp/to_sub/docs/index.asciidoc
echo "extra extra extra" >> /tmp/to_sub/docs/index.asciidoc

/docs_build/build_docs.pl --in_standard_docker --all --push \
--target_repo /tmp/keep_hash_and_sub_dir.git \
--conf two_repos_conf.yaml \
--keep_hash --sub_dir source1:master:/tmp/to_sub | tee /tmp/out
$(call GREP,'Pushing changes',/tmp/out)

git clone /tmp/keep_hash_and_sub_dir.git /tmp/keep_hash_and_sub_dir
$(call GREP,'extra extra extra',/tmp/keep_hash_and_sub_dir/html/test/current/_chapter.html)


define GREP=
# grep for a string in a file, outputting the whole file if there isn't
# a match.
[ -e $(2) ] || { \
echo "can't find $(2)"; \
ls $$(dirname $(2)); \
false; \
}
grep $(1) $(2) > /dev/null || { \
echo "Couldn't find $(1) in $(2):"; \
cat $(2); \
false; \
}
endef

define BUILD_MINIMAL_ALL=
# Builds `--all` docs using a "minimal" source file

# First build a repository to use as the source.
$(call INIT_REPO_WITH_FILE,/tmp/source,minimal.asciidoc,index.asciidoc)

# Initialize a bare repository that the docs build process can use as a
# remote. It is used to pushing to github but it can push to a remote on
# the filesystem just fine.
git init --bare /tmp/small_all.git
rm -rf $(1).git
git init --bare $(1).git

# Actually build the docs
/docs_build/build_docs.pl --in_standard_docker --all --push \
--target_repo /tmp/small_all.git \
--target_repo $(1).git \
--conf small_conf.yaml
endef

define MINIMAL_ALL_EXPECTED_FILES=
# Checks that $(1).git contains the expected result of building the
# "minimal" source file
git clone $(1).git $(1)
[ -s $(1)/html/branches.yaml ]
$(call GREP,'<a class="ulink" href="test/current/index.html" target="_top">Test book</a>',$(1)/html/index.html)
$(call GREP,'<meta http-equiv="refresh" content="0; url=current/index.html">',$(1)/html/test/index.html)
[ -s $(1)/html/test/current/index.html ]
endef

# Check out the files we just built
git clone /tmp/small_all.git /tmp/small_all
define INIT_REPO_WITH_FILE=
# Initializes the repo at $(1) and commits the file in $(2) with the
# name $(3)
rm -rf $(1)
git init $(1)
mkdir -p $$(dirname $(1)/$(3))
cp $(2) $(1)/$(3)
cd $(1) && \
git add . && \
git commit -m 'init'
endef

define GREP=
# grep for a string in a file, outputting the whole file if there isn't
Expand All @@ -162,4 +283,4 @@ define GREP=
cat $(2); \
false; \
}
endef
endef
9 changes: 9 additions & 0 deletions integtest/includes_source2.asciidoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
= Title

== Chapter

I include simple between here

include::../../source2/index.asciidoc[]

and here.
4 changes: 2 additions & 2 deletions integtest/small_conf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ contents:
prefix: test
current: master
branches: [ master ]
index: minimal.asciidoc
index: index.asciidoc
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.asciidoc allows me to reuse this file in other tests.

tags: test tag
subject: Test
sources:
-
repo: source
path: .
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

. isn't a thing we ever see in the real conf.yaml so I replaced it here.

path: index.asciidoc
46 changes: 46 additions & 0 deletions integtest/two_repos_conf.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# This conf file builds a book using /tmp/source1 and /tmp/source2. See
# small_conf.yaml for description of the sections.

template:
path: .template/
branch:
default:
base_url: 'https://www.elastic.co/'
template_url: 'https://www.elastic.co/guide_template'
staging:
base_url: 'https://stag-www.elastic.co/'
template_url: 'https://stag-www.elastic.co/guide_template'
defaults:
POSTHEAD: |
<link rel="stylesheet" type="text/css" href="styles.css" />
FINAL: |
<script type="text/javascript" src="docs.js"></script>
<script type='text/javascript' src='https://cdn.rawgit.com/google/code-prettify/master/loader/run_prettify.js?lang=yaml'></script>

paths:
build: html/
branch_tracker: html/branches.yaml
repos: /tmp/repos/

repos:
source1: /tmp/source1
source2: /tmp/source2

contents_title: Elastic Stack and Product Documentation

contents:
-
title: Test book
prefix: test
current: master
branches: [ master ]
index: docs/index.asciidoc
tags: test tag
subject: Test
sources:
-
repo: source1
path: docs
-
repo: source2
path: index.asciidoc
2 changes: 1 addition & 1 deletion lib/ES/Book.pm
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ sub _build_book {
&& !$template->md5_changed($branch_dir)
&& !$source->has_changed( $self->title, $branch, $self->asciidoctor );

my ( $checkout, $edit_urls, $first_path ) = $source->prepare($branch);
my ( $checkout, $edit_urls, $first_path ) = $source->prepare($self->title, $branch);

$pm->start($branch) and return;
say " - Branch: $branch - Building...";
Expand Down
Loading