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

Update the build pipeline #386

Merged
merged 26 commits into from
Jun 18, 2024
Merged

Conversation

dmint789
Copy link
Member

@dmint789 dmint789 commented May 12, 2024

This is a big PR, but this was a very important update for us to make. It should improve the developer experience greatly when it comes to building the PDFs to see a preview of the documents, which was previously only really easy to do by creating a PR and waiting for the Github pipeline to do it, which always took a while and made it harder to iterate with minor adjustments. This PR does the following:

  • Update the build script to use weasyprint instead of wkhtmltopdf, which is no longer maintained and causes bugs in the output PDFs sometimes. Some other minor improvements to this script have also been made.
  • Add a new build script that uses Docker for consistent file rendering.
  • Unify the duplicated styles between the documents and edudoc stylesheets, and keep the directory-specific styles in their own separate files: documents-style.css and edudoc-style.css
  • Make minor adjustments to the documents themselves to ensure there are no alignment issues and to make some of the Markdown look cleaner.
  • Renumber points 5. and 6. of the WCA Bylaws section 2.4 as 4. and 5. respectively (point 4. was missing). Keep in mind that it's displayed as 1-5 in production rn, because Markdown seems to ignore what number you actually put there and simply uses the correct one in order.

Copy link
Contributor

@nsilvestri nsilvestri left a comment

Choose a reason for hiding this comment

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

Inconsistencies noticed:

  1. Slight stylistic differences: spacing between lines in particular. Causes some paragraphs to move to other pages.
  2. Styling on example email usage in Staff Crash Course

Document issues:

The only issue I found was that on some bullet points and numbers, the bullet/number is on a line above the text for that bullet/number. Non-exhaustive examples are:

  • Bylaws doc: 2.4, point 5
  • WCRP Announcement Criteria 1., 1.11, 2.5.4.1
  • Competitor tutorial, Important Definitions

bin/docker_build.sh Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dmint789
Copy link
Member Author

@nsilvestri could you provide an example of an unwanted page break due to the spacing between lines? For me the spacing looks the same as it does in production rn.

@dmint789
Copy link
Member Author

Staff crash course fixed

@nsilvestri
Copy link
Contributor

@dmint789 the page breaks caused by spacing are not "unwanted", just slightly different. 1-2 lines will be pushed to the next page, or vice versa. Very minor stylistic thing, just noting it as an inconsistency.

@dmint789
Copy link
Member Author

Fixed the bullet point issue in the competitor tutorial

@dmint789
Copy link
Member Author

dmint789 commented May 15, 2024

Okay, now I need to update the Github actions. I think I've fixed all of the issues you found.

@dmint789
Copy link
Member Author

Found another issue: the counters reset when there is content between two items (see the competition templates doc for an example). Currently working on fixing this.

@dmint789
Copy link
Member Author

Fixed

@dmint789
Copy link
Member Author

I have personally gone through every single markdown document in the repository, fixed all alignment issues and made sure they all look almost exactly the same as the original docs rendered with wkhtmltopdf. The PR is now ready for review.

@nsilvestri could you please review the changes in the documents and edudoc folders? It's almost exclusively alignment fixes and changes from pure HTML to markdown/pandoc syntax in some of our old docs. My expectation is that you won't find any issues, as I tried being as thorough as possible, but it's good to have another set of eyes on the changes.

@thewca/software-team could you please review the changes in the root of the repo? Namely, the changes to the assets and bin folders, the README and my newly-created Dockerfile.

I will also review all of the aforementioned changes myself for good measure.

@dmint789 dmint789 marked this pull request as ready for review May 19, 2024 11:09
@dmint789 dmint789 requested a review from a team as a code owner May 19, 2024 11:09
@dmint789
Copy link
Member Author

@thewca/software-team also, I'll need someone to override the PR build test, cause it's failing due to not being up to date with these changes

nsilvestri
nsilvestri previously approved these changes May 21, 2024
Copy link
Contributor

@nsilvestri nsilvestri left a comment

Choose a reason for hiding this comment

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

Thanks for seeing through this colossal effort. LGTM on behalf of WQAC.

Copy link
Member

@gregorbg gregorbg left a comment

Choose a reason for hiding this comment

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

This review includes the technical scripts only. I have not reviewed every single md file nor have I meticulously verified the generated PDF contents. That is left entirely up to WQAC.

Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
assets/edudoc-style.css Show resolved Hide resolved
bin/build.sh Show resolved Hide resolved
bin/build.sh Show resolved Hide resolved
bin/build.sh Outdated Show resolved Hide resolved
bin/build.sh Show resolved Hide resolved
bin/build.sh Show resolved Hide resolved
bin/docker_build.sh Outdated Show resolved Hide resolved
bin/install_dependencies.sh Outdated Show resolved Hide resolved
@dmint789
Copy link
Member Author

dmint789 commented Jun 6, 2024

@gregorbg is this good to be merged now?

bin/build.sh Outdated Show resolved Hide resolved
documents-guide.md Show resolved Hide resolved
documents-guide.md Show resolved Hide resolved
documents/minutes/2017-12-12 WCA Board Founding Meeting.md Outdated Show resolved Hide resolved
bin/docker_build.sh Show resolved Hide resolved
bin/docker_build.sh Show resolved Hide resolved
@dmint789
Copy link
Member Author

Looks like all issues are resolved now

gregorbg
gregorbg previously approved these changes Jun 13, 2024
Copy link
Member

@gregorbg gregorbg left a comment

Choose a reason for hiding this comment

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

The technical build scripts have my approval. I did not (and will not) check the final rendered output of every single document in the new pipeline.

Sidenote: The CI error log looks like you forgot sudo somewhere

@dmint789
Copy link
Member Author

Oh, maybe it's the apt commands. Also, does the build pipeline use the setup files from master or from this branch I'm trying to merge? I assumed the CI error would never go away until this PR is merged.

@dmint789
Copy link
Member Author

@thewca/board can you please approve? This probably requires Board approval, because I had to edit many of the documents themselves, but the only real change is the last point in the description of this PR, but it doesn't even affect the rendered file, since the numbers themselves are ignored. I just wanted to fix the numbering in the Bylaws Markdown. I have personally checked the rendering of every single document in this repo, and everything has remained consistent, bar some very minor alignment here and there.

@dmint789
Copy link
Member Author

@gregorbg Looks like I've answered my own question, as the PR build test has a different output after my commit now. I'll try to fix it, looks like the repos used by the github action are different from the ubuntu image repos.

@dmint789
Copy link
Member Author

@gregorbg according to this, package version locking is not possible on ubuntu. I'd just remove the pandoc and weasyprint package versions and call it a day, unless you know of a way to get a specific version of these packages from apt.

nsilvestri
nsilvestri previously approved these changes Jun 15, 2024
Copy link
Contributor

@nsilvestri nsilvestri left a comment

Choose a reason for hiding this comment

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

Approved on behalf as Board regarding non-content or formatting changes to Board-sensitive documents.

@dmint789
Copy link
Member Author

@gregorbg the checks are finally passing. Can you please re-approve, cause the new commits made your approval stale. Maybe we can revisit package version locking in another issue and PR, cause this is currently blocking two of our projects.

@nsilvestri nsilvestri self-requested a review June 16, 2024 17:39
nsilvestri
nsilvestri previously approved these changes Jun 16, 2024
@nsilvestri nsilvestri self-requested a review June 16, 2024 17:43
@nsilvestri nsilvestri dismissed their stale review June 16, 2024 17:44

pre-emptive

@dmint789 dmint789 merged commit 03eb3bc into thewca:master Jun 18, 2024
1 check passed
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.

None yet

3 participants