Fix PDF generation, exit code from make and parallelization by lzap · Pull Request #1002 · theforeman/foreman-documentation

lzap · 2022-01-19T13:29:47Z

So it turns out we have several issues.

Asciidoctor PDF now has a missing dependency "matrix". Without this gem, PDF generation simply fails, probably a bug usptream.

Second problem actually hid the first one - I added asciidoctor ... | tee LOGFILE; grep LOGFILE to search for warnings which were never returned via exit code by asciidoctor, but I haven't realized that the pipe always returns zero exit code so even when HTML/PDF generation was broken (we had brokens links), makefile was still returning zero.

After I fixed the tee problem, a third appeared - recursive make implementation did not care about return codes from the submakes at all, therefore when targets "all-html" or "all-pdf" were called, it always returned zero even if we had issues (warnings via the grep command).

Finally, I am making small improvement - for PR CI runs, let's simply build just one PDF to speed things up just to see if it actually builds. For deployments, let's build all of them, this creates a huge ZIP file and takes much longer. I think this is a good solution, previously we were building no PDFs for PR CI runs.

The root cause for PDF generation failing was because I was giving it the wrong images directory. I am going to fix all problems in separate commits. On top of that, since I am looking into this, I reimplemented the parallel make after reading GNU Makefile manual now in a proper way so it is -j friendly. Now we can use it on CI to even further speedups builds, for example PDF generation on my Intel NUC 8thgen:

# make clean; make html
real	0m14,125s
user	0m12,568s
sys	0m1,552s

# make clean; make html -j5
real	0m2,578s
user	0m8,955s
sys	0m0,897s

# make clean; make pdf
real	0m49,084s
user	0m46,572s
sys	0m2,375s

# make clean; make pdf -j5
real	0m23,316s
user	1m16,476s
sys	0m3,601s

For parallelization, I had to drop the clunky images symlinking, now we use simply cp to copy images over to the destination dir for each guide. I actually have a hard-link option to make it even faster. I also had to solve the common/images vs images/ directories problem which I run into when I was working on #977. We used to have images/ subdirectory for some guides, now we have it for all of them and images are copied from both directories into a single one.

Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>

lzap · 2022-01-20T09:31:29Z

So added make -j3 to GHA (builders do appear to have 2 cores according to documentation). Previous run was running 2m49s after the change it was 2m24s. Not a huge difference, for PDFs it is similar: 1m18s vs 52s.

Anyway, my main goal was to fix how exit codes are handled and also PDFs were not generating at all.

lzap · 2022-01-20T09:33:43Z

.github/workflows/deploy.yml

-      github.repository_owner == 'theforeman' &&
-      (github.ref == 'refs/heads/master' ||
-      startsWith(github.ref, 'refs/heads/2.') ||
-      startsWith(github.ref, 'refs/heads/3.'))


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

lzap · 2022-01-20T09:33:59Z

Gemfile

 gem 'asciidoctor'
 gem 'asciidoctor-pdf'
+# missing dependency for asciidoctor-pdf
+gem 'matrix'


This was causing Ruby to fail.

lzap · 2022-01-20T09:34:37Z

guides/Makefile

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


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

lzap · 2022-01-20T09:34:56Z

guides/Makefile

-		$(MAKE) --directory="$$DIR" clean ; \
-	done
+serve:
+	python -m http.server --directory ./build 8813


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

lzap · 2022-01-20T09:36:46Z

guides/common/Makefile

-$(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 $@


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.

lzap · 2022-01-20T09:37:09Z

guides/common/Makefile

-	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)


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

lzap · 2022-01-20T09:37:37Z

guides/common/Makefile


 $(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


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

ekohl

Looks good to me.

lzap · 2022-01-20T09:38:34Z

guides/doc-Administering_with_Ansible/master.adoc

@@ -1,5 +1,5 @@
 include::common/attributes.adoc[]
-:imagesdir: common/images
+:imagesdir: images


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.

lzap marked this pull request as draft January 19, 2022 13:30

lzap force-pushed the pdf-ruby3-fix branch 2 times, most recently from f0e5b3c to a5ce20e Compare January 19, 2022 14:09

Add missing asciidoctor-pdf dependency

b09cab2

lzap force-pushed the pdf-ruby3-fix branch 3 times, most recently from 14769b7 to 34396e3 Compare January 20, 2022 08:02

lzap changed the title ~~Test Ruby 3.1 PDF generation~~ Fix PDF generation and exit code from make Jan 20, 2022

lzap force-pushed the pdf-ruby3-fix branch from 34396e3 to 624a34f Compare January 20, 2022 08:10

lzap changed the title ~~Fix PDF generation and exit code from make~~ Fix PDF generation, exit code from make and parallelization Jan 20, 2022

lzap marked this pull request as ready for review January 20, 2022 09:22

lzap force-pushed the pdf-ruby3-fix branch from 624a34f to 904ca40 Compare January 20, 2022 09:22

Fail properly on errors and -j friendly

fd40c21

Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>

lzap force-pushed the pdf-ruby3-fix branch from 904ca40 to fd40c21 Compare January 20, 2022 09:26

lzap commented Jan 20, 2022

View reviewed changes

ekohl approved these changes Jan 20, 2022

View reviewed changes

lzap commented Jan 20, 2022

View reviewed changes

lzap merged commit 23a6a71 into theforeman:master Jan 20, 2022

lzap deleted the pdf-ruby3-fix branch January 20, 2022 09:42

ekohl mentioned this pull request Dec 5, 2025

Use a common CSS style file for all build flavors #4517

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Fix PDF generation, exit code from make and parallelization#1002
lzap merged 2 commits intotheforeman:masterfrom
lzap:pdf-ruby3-fix

lzap commented Jan 19, 2022 •

edited

Loading

Uh oh!

lzap commented Jan 20, 2022

Uh oh!

lzap Jan 20, 2022

Uh oh!

lzap Jan 20, 2022

Uh oh!

lzap Jan 20, 2022

Uh oh!

lzap Jan 20, 2022

Uh oh!

lzap Jan 20, 2022

Uh oh!

lzap Jan 20, 2022

Uh oh!

lzap Jan 20, 2022

Uh oh!

ekohl left a comment

Uh oh!

lzap Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lzap commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzap commented Jan 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lzap commented Jan 19, 2022 •

edited

Loading