Skip to content

Refactor book:build task #1598

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

Merged
merged 18 commits into from
Feb 11, 2021
Merged

Refactor book:build task #1598

merged 18 commits into from
Feb 11, 2021

Conversation

bagasme
Copy link
Contributor

@bagasme bagasme commented Jan 28, 2021

Changes

This PR refactors monolithic book:build task into several smaller tasks:

  1. book/contributors.txt file task: generates contributors list required to build book formats
  2. book:build_html - build HTML format
  3. book:build_epub - build Epub format
  4. book:build_pdf - build PDF format
  5. book:check - check HTML and Epub

This also add convenience tasks:

  1. book:clean - clean artifacts (contributors list and books)
  2. book:ci - build books with checking enforced (not ignored), targeted for CI.

Note: The CI doesn't execute book:ci, but still running book:build instead. Discussions are welcome whether the CI should switch to book:ci or not.

Context

Fixes #1595

This preliminary commit move build-related variables from :build task
out to :book namespace. Such variables will be referenced by each
generating tasks.

Signed-off-by: Bagas Sanjaya <[email protected]>
For each formats, including currently unsupported Mobi format, extract
from :build task.

Signed-off-by: Bagas Sanjaya <[email protected]>
This file task is required to build books.

Signed-off-by: Bagas Sanjaya <[email protected]>
The default task is now empty, so we can simply list its dependency
tasks.

Signed-off-by: Bagas Sanjaya <[email protected]>
Clean all generated artifacts (contributors list and book formats).

Signed-off-by: Bagas Sanjaya <[email protected]>
As suggested by @jnavila.

Check only HTML and epub books. As before this refactor, default
book:build task also check them.

Signed-off-by: Bagas Sanjaya <[email protected]>
Instead of invoking :check as dependency, explicitly invoke it inside
the body of :build.

As suggested by @jnavila, any errors raised from :check should not cause
FTBFS, thus ignore them.

Signed-off-by: Bagas Sanjaya <[email protected]>
As suggested by @jnavila.

Similar to :build, but don't ignore any errors.

Signed-off-by: Bagas Sanjaya <[email protected]>
Copy link
Contributor

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

What do you think of these changes?

Apply suggestions from @HonkingGoose:

  - README rewording
  - FIXME line fitting on Rakefile

Co-authored-by: HonkingGoose <[email protected]>
@bagasme
Copy link
Contributor Author

bagasme commented Feb 6, 2021

review ping

@HonkingGoose
Copy link
Contributor

@ben @jnavila could you take a look at this?

Copy link
Member

@jnavila jnavila left a comment

Choose a reason for hiding this comment

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

Thank you for proposing. These are mostly style comments.

As per code review, @jnavila reported an error that requires begin/end
block for multiline rescue body.

Signed-off-by: Bagas Sanjaya <[email protected]>
As per code review by @jnavila

Signed-off-by: Bagas Sanjaya <[email protected]>
As requested by @jnavila in code review.

Signed-off-by: Bagas Sanjaya <[email protected]>
In order to check contributors list by commit hash, the hash have to be
added to the list.

Signed-off-by: Bagas Sanjaya <[email protected]>
…it hash

As requested by @jnavila in code review.

Compare current HEAD commit hash against the hash stored in the header
of contributors list. If these match, go on. If not, refresh (rm and
generate contributors list again).

Signed-off-by: Bagas Sanjaya <[email protected]>
As to be consistent with other non-interpolated strings.

Signed-off-by: Bagas Sanjaya <[email protected]>
@bagasme
Copy link
Contributor Author

bagasme commented Feb 10, 2021

review ping

Rakefile Outdated
Comment on lines 18 to 37
# Check contributors list
# This checks commit hash stored in the header of list against current HEAD
def check_contrib
if File.exist?('book/contributors.txt')
current_head_hash = `git rev-parse --short HEAD`.strip
header = `head -n 1 book/contributors.txt`.strip
# Match regex, then coerce resulting array to string by join
header_hash = header.scan(/[a-f0-9]{7,}/).join

if header_hash == current_head_hash
puts "Hash on header of contributors list (#{header_hash}) matches the current HEAD (#{current_head_hash})"
else
puts "Hash on header of contributors list (#{header_hash}) does not match the current HEAD (#{current_head_hash}), refreshing"
`rm book/contributors.txt`
# Reenable and invoke task again
Rake::Task['book/contributors.txt'].reenable
Rake::Task['book/contributors.txt'].invoke
end
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Check contributors list
# This checks commit hash stored in the header of list against current HEAD
def check_contrib
if File.exist?('book/contributors.txt')
current_head_hash = `git rev-parse --short HEAD`.strip
header = `head -n 1 book/contributors.txt`.strip
# Match regex, then coerce resulting array to string by join
header_hash = header.scan(/[a-f0-9]{7,}/).join
if header_hash == current_head_hash
puts "Hash on header of contributors list (#{header_hash}) matches the current HEAD (#{current_head_hash})"
else
puts "Hash on header of contributors list (#{header_hash}) does not match the current HEAD (#{current_head_hash}), refreshing"
`rm book/contributors.txt`
# Reenable and invoke task again
Rake::Task['book/contributors.txt'].reenable
Rake::Task['book/contributors.txt'].invoke
end
end
end
# Check contributors list
# This checks the commit hash stored in the header of the list against the current HEAD
def check_contrib
if File.exist?('book/contributors.txt')
current_head_hash = `git rev-parse --short HEAD`.strip
header = `head -n 1 book/contributors.txt`.strip
# Match regex, then coerce resulting array to string by join
header_hash = header.scan(/[a-f0-9]{7,}/).join
if header_hash == current_head_hash
puts "Hash on header of contributors list (#{header_hash}) matches the current HEAD (#{current_head_hash})"
else
puts "Hash on header of contributors list (#{header_hash}) does not match the current HEAD (#{current_head_hash}), refreshing"
`rm book/contributors.txt`
# Re-enable and invoke task again
Rake::Task['book/contributors.txt'].reenable
Rake::Task['book/contributors.txt'].invoke
end
end
end

Some small comment fixes.

@jnavila
Copy link
Member

jnavila commented Feb 10, 2021

The build is failing with syntax error.

That line cause FTBFS on CI. It was forgotten to be deleted on previous
commit, due to hurry.

Signed-off-by: Bagas Sanjaya <[email protected]>
@bagasme
Copy link
Contributor Author

bagasme commented Feb 11, 2021

review ping

@jnavila
Copy link
Member

jnavila commented Feb 11, 2021

It looks good to me. Thank you very much!

@jnavila jnavila merged commit 9620145 into progit:master Feb 11, 2021
@bagasme bagasme deleted the build-task-refactor branch February 12, 2021 09:25
Copy link

@shi1647ra shi1647ra left a comment

Choose a reason for hiding this comment

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

srgizh added a commit to srgizh/progit2-ru that referenced this pull request Feb 28, 2021
srgizh added a commit to srgizh/progit2-ru that referenced this pull request Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactoring book:build
4 participants