Skip to content

Fix PDF generation, exit code from make and parallelization #1002

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.

Sign up for GitHub

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

Merged
lzap merged 2 commits into from
Jan 20, 2022
Merged

Fix PDF generation, exit code from make and parallelization #1002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b09cab2
Add missing asciidoctor-pdf dependency
lzap Jan 20, 2022
fd40c21
Fail properly on errors and -j friendly
lzap Jan 20, 2022
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
46 changes: 25 additions & 21 deletions .github/workflows/deploy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ env:
RUBY_VERSION: 3.1
PYTHON_VERSION: 2.x
BUNDLE_WITHOUT: "nanoc"
MAKE_J: 3

jobs:
check-html:
Expand Down Expand Up @@ -55,11 +56,11 @@ jobs:

- name: Build HTML with local links
run: |
make html BUILD=foreman-el LINKS=LOCAL
make html BUILD=foreman-deb LINKS=LOCAL
make html BUILD=katello LINKS=LOCAL
make html BUILD=satellite LINKS=LOCAL
make html BUILD=orcharhino LINKS=LOCAL
make -j ${{ env.MAKE_J }} html BUILD=foreman-el LINKS=LOCAL
make -j ${{ env.MAKE_J }} html BUILD=foreman-deb LINKS=LOCAL
make -j ${{ env.MAKE_J }} html BUILD=katello LINKS=LOCAL
make -j ${{ env.MAKE_J }} html BUILD=satellite LINKS=LOCAL
make -j ${{ env.MAKE_J }} html BUILD=orcharhino LINKS=LOCAL

- name: Check HTML links of all builds
run: |
Expand Down Expand Up @@ -94,11 +95,11 @@ jobs:
- name: Build HTML with real links
run: |
make clean
make html BUILD=foreman-el
make html BUILD=foreman-deb
make html BUILD=katello
make html BUILD=satellite
make html BUILD=orcharhino
make -j ${{ env.MAKE_J }} html BUILD=foreman-el
make -j ${{ env.MAKE_J }} html BUILD=foreman-deb
make -j ${{ env.MAKE_J }} html BUILD=katello
make -j ${{ env.MAKE_J }} html BUILD=satellite
make -j ${{ env.MAKE_J }} html BUILD=orcharhino

- name: Upload HTML
uses: actions/upload-artifact@v2
Expand All @@ -107,11 +108,6 @@ jobs:
path: guides/build/

build-pdf:
if: >
github.repository_owner == 'theforeman' &&
(github.ref == 'refs/heads/master' ||
startsWith(github.ref, 'refs/heads/2.') ||
startsWith(github.ref, 'refs/heads/3.'))
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

I moved this down, we now build one PDF version for PR CI and all for deploy commits.

runs-on: ubuntu-latest
defaults:
run:
Expand Down Expand Up @@ -140,13 +136,21 @@ jobs:
- name: Clean the environment
run: make clean

- name: Build PDFs
- name: Build PDFs (master/stable)
run: |
make -j ${{ env.MAKE_J }} pdf BUILD=foreman-el

- name: Build PDFs (PR)
if: >
github.repository_owner == 'theforeman' &&
(github.ref == 'refs/heads/master' ||
startsWith(github.ref, 'refs/heads/2.') ||
startsWith(github.ref, 'refs/heads/3.'))
run: |
make pdf BUILD=foreman-el
make pdf BUILD=foreman-deb
make pdf BUILD=katello
make pdf BUILD=satellite
make pdf BUILD=orcharhino
make -j ${{ env.MAKE_J }} pdf BUILD=foreman-deb
make -j ${{ env.MAKE_J }} pdf BUILD=katello
make -j ${{ env.MAKE_J }} pdf BUILD=satellite
make -j ${{ env.MAKE_J }} pdf BUILD=orcharhino

- name: Upload PDFs
uses: actions/upload-artifact@v2
Expand Down
2 changes: 2 additions & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ source 'https://rubygems.org'

gem 'asciidoctor'
gem 'asciidoctor-pdf'
# missing dependency for asciidoctor-pdf
gem 'matrix'
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

This was causing Ruby to fail.


gem 'compass', '0.12.7'
gem 'zurb-foundation', '4.3.2'
38 changes: 15 additions & 23 deletions guides/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,29 @@
SHELL := /bin/bash
ALL_DIRS = $(shell ls -d doc-*)
GUIDES-HTML = $(ALL_DIRS)
SUBDIRS = $(shell ls -d doc-*)

.PHONY: all html pdf linkchecker all-html all-pdf clean all-clean linkchecker-tryer $(GUIDES-HTML)
.PHONY: all clean html pdf linkchecker linkchecker-tryer serve subdirs $(SUBDIRS)

all: html

html: all-html
html: subdirs

pdf: all-pdf
pdf: subdirs

clean: all-clean
subdirs: $(SUBDIRS)

$(SUBDIRS):
$(MAKE) -C "$@" $(MAKECMDGOALS)

clean:
@for DIR in $(SUBDIRS) ; do \
$(MAKE) -s --directory="$$DIR" clean ; \
done
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

This is completely simplified, it now properly calls make so recursive option does work.


linkchecker:
find build -type f -name index\*html | xargs linkchecker -r1 -f common/linkchecker.ini --check-extern

linkchecker-tryer:
find build -type f -name index\*html | xargs linkchecker -r1 -f common/linkchecker.ini --check-extern | linkchecker-tryer

all-html: $(GUIDES-HTML)

$(GUIDES-HTML):
$(MAKE) --directory=$@ html

all-pdf:
for DIR in $(ALL_DIRS) ; do \
$(MAKE) --directory="$$DIR" pdf ; \
done

$(GUIDES-PDF):
$(MAKE) --directory=$@ pdf

all-clean:
for DIR in $(ALL_DIRS) ; do \
$(MAKE) --directory="$$DIR" clean ; \
done
serve:
python -m http.server --directory ./build 8813
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

The same target is in the common Makefile, I use this for testing.

21 changes: 10 additions & 11 deletions guides/common/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,22 @@ ifeq ($(LINKS), LOCAL)
BASEURL_ATTR = "-a BaseURL=file://$(BUILD_DIRA)"
endif

.SHELLFLAGS = -o pipefail -c
.PHONY = all prepare css html pdf clean browser server linkchecker open-pdf

all: html

prepare:
@mkdir -p $(BUILD_DIR) $(DEST_DIR) $(IMAGES_DIR) $(IMAGES_DIR) $(DEST_DIR)/common/images
@mkdir -p $(BUILD_DIR) $(DEST_DIR) $(IMAGES_DIR) $(DEST_DIR)/sass-cache

css: prepare $(DEST_DIR)/$(BUILD).css

html: css $(IMAGES_TS) $(DEST_HTML)

pdf: prepare $(DEST_PDF)
pdf: prepare $(IMAGES_TS) $(DEST_PDF)

clean:
@rm -rf "$(DEST_DIR)" "$(DEST_PDF)" "$(BUILD_DIR)/default.css"
@rm -rf "$(DEST_DIR)" "$(DEST_PDF)"

browser: html
${BROWSER_OPEN} "file://$(realpath $(ROOTDIR)/$(DEST_HTML))"
Expand All @@ -57,15 +60,11 @@ linkchecker: html
open-pdf: pdf
${BROWSER_OPEN} "$(realpath $(ROOTDIR)/$(DEST_PDF))"

$(BUILD_DIR)/default.css: $(CSS_SOURCES)
bundle exec sass -C --style $(CSS_STYLE) -I $(COMMON_DIR)/css $(COMMON_DIR)/css/default.scss $@

$(DEST_DIR)/$(BUILD).css: $(BUILD_DIR)/default.css
cp $< $@
$(DEST_DIR)/$(BUILD).css: $(CSS_SOURCES)
bundle exec sass --cache-location $(DEST_DIR)/sass-cache --style $(CSS_STYLE) -I $(COMMON_DIR)/css $(COMMON_DIR)/css/default.scss $@
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

Because sass command is a bit slow, previously it built into a common BUILD_DIR just once and other guides could simply copy the generated file quickly over to the destination. With parallel option this is no longer possible (I was hitting problems). So now sass needs to be executed for each guide separately, which is a bit slower. I implemented cache option, but that does not speeds things up on CI (where we start from a clean state anyway). Later on, we can migrate to Dart-SCSS.


$(IMAGES_TS): $(IMAGES)
cp $? $(IMAGES_DIR)/
pushd "$(IMAGES_DIR)" >/dev/null && for IMG in *.*; do ln -fs ../../images/$$IMG ../common/images/$$IMG; done && popd >/dev/null
cp -lrf $? $(IMAGES_DIR)
@touch $(IMAGES_TS)
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

This was dodgy, now images are properly copied (if they are new).


$(DEST_HTML): $(SOURCES) $(DEPENDENCIES) $(DOCINFO_SOURCES)
Expand All @@ -76,6 +75,6 @@ $(DEST_HTML): $(SOURCES) $(DEPENDENCIES) $(DOCINFO_SOURCES)
@! grep -RE '<a href="#[a-zA-Z0-9_-]+">\[[a-zA-Z0-9_-]+\]</a>' $@

$(DEST_PDF): $(SOURCES) $(IMAGES) $(DEPENDENCIES)
bundle exec asciidoctor-pdf -a build=$(BUILD) -d book --trace -o $@ $< 2>&1 | tee $@.log
bundle exec asciidoctor-pdf -a build=$(BUILD) -a imagesdir=$(IMAGES_DIR) -d book --trace -o $@ $< 2>&1 | tee $@.log
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

This was another issue with PDF generation - it could not find images in the correct directory. This fixes it.

@echo CHECKING FOR ASCIIDOCTOR-PDF ERRORS AND WARNINGS
@! grep -Eq "^asciidoctor: (ERROR|WARNING)" $@.log
2 changes: 1 addition & 1 deletion guides/doc-Administering_with_Ansible/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
include::common/attributes.adoc[]
:imagesdir: common/images
:imagesdir: images
Copy link
Member Author

@lzap lzap Jan 20, 2022

Choose a reason for hiding this comment

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

This also consolidates where images should be find - they are always copied by make into the BUILD_DIR/images so no longer we will confuse common and non-common images. They are all copied to the same destination, of course locals overriding common.

include::common/header.adoc[]

:configuring-ansible:
Expand Down
2 changes: 1 addition & 1 deletion guides/doc-Configuring_Ansible/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
include::common/attributes.adoc[]
:imagesdir: common/images
:imagesdir: images
include::common/header.adoc[]

:configuring-ansible:
Expand Down
2 changes: 1 addition & 1 deletion guides/doc-Installing_Proxy_on_Debian/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
:imagesdir: common/images
:imagesdir: images
:installing-capsule-server:
:numbered:
:toc:
Expand Down
2 changes: 1 addition & 1 deletion guides/doc-Installing_Proxy_on_Red_Hat/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
:imagesdir: common/images
:imagesdir: images
:numbered:
ifndef::satellite[]
:toc:
Expand Down
2 changes: 1 addition & 1 deletion guides/doc-Installing_Satellite_Server_Disconnected/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
include::common/attributes.adoc[]
:imagesdir: common/images
:imagesdir: images
:installing-satellite-server-disconnected:
include::common/header.adoc[]
:context: satellite
Expand Down
2 changes: 1 addition & 1 deletion guides/doc-Installing_Server_on_Debian/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
:imagesdir: common/images
:imagesdir: images
:numbered:
:toc:
:toc-placement: left
Expand Down
2 changes: 1 addition & 1 deletion guides/doc-Installing_Server_on_Red_Hat/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
include::common/attributes.adoc[]
:imagesdir: common/images
:imagesdir: images
:numbered:
ifdef::foreman-el,foreman-deb,katello[]
:toc:
Expand Down
2 changes: 1 addition & 1 deletion guides/doc-Quickstart_Guide/master.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
include::common/attributes.adoc[]
:imagesdir: common/images
:imagesdir: images
:numbered:
ifdef::foreman-el,foreman-deb,katello[]
:toc:
Expand Down