Use a common CSS style file for all build flavors #4517
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this we created a CSS file for each build flavor, but with the same inputs. This duplication isn't needed so aligning on a single style.css leads to a quicker build and fewer files.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideConsolidates CSS generation for all build flavors to a single shared style.css and updates the HTML build target to depend on and reference that common stylesheet instead of flavor-specific CSS files. Sequence diagram for HTML build with common stylesheetsequenceDiagram
participant Dev as Developer
participant Make as Make
participant Sass as Sass
participant FS as Filesystem
participant Ascii as Asciidoctor
Dev->>Make: run make DEST_HTML
Make->>Sass: build DEST_DIR/style.css from CSS_SOURCES
Sass->>FS: write DEST_DIR/style.css
FS-->>Make: DEST_DIR/style.css available
Make->>FS: ensure IMAGES_TS and images in IMAGES_DIR
FS-->>Make: IMAGES_TS up to date
Make->>Ascii: build DEST_HTML
Note over Make,Ascii: Pass attribute stylesheet=DEST_DIR/style.css
Ascii->>FS: read SOURCES, DEPENDENCIES, DOCINFO_SOURCES
Ascii->>FS: read DEST_DIR/style.css
Ascii->>FS: write DEST_HTML and DEST_HTML.log
FS-->>Make: DEST_HTML created
Make-->>Dev: build finished with shared stylesheet
Flow diagram for shared CSS generation in the build processflowchart LR
subgraph Inputs
CSS_SOURCES["CSS_SOURCES"]
SOURCES["SOURCES"]
DEPENDENCIES["DEPENDENCIES"]
DOCINFO_SOURCES["DOCINFO_SOURCES"]
IMAGES["IMAGES"]
end
CSS_SOURCES --> StyleTarget["DEST_DIR/style.css target"]
StyleTarget --> StyleCSS["DEST_DIR/style.css"]
IMAGES --> ImagesTarget["IMAGES_TS target"]
ImagesTarget --> IMAGES_TS["IMAGES_TS"]
SOURCES --> HTMLTarget["DEST_HTML target"]
DEPENDENCIES --> HTMLTarget
DOCINFO_SOURCES --> HTMLTarget
StyleCSS --> HTMLTarget
IMAGES_TS --> HTMLTarget
HTMLTarget --> DEST_HTML["DEST_HTML (HTML output)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
Needs tech review
11 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting
style.cssinto a Make variable (e.g.STYLESHEET_NAME) so the filename remains configurable and avoids scattering a hard-coded name across the build logic. - If multiple flavors can be built concurrently, using a single
$(DEST_DIR)/style.csstarget may introduce race conditions; it might be safer to either make the rule.PHONYor ensure flavor builds serialize the stylesheet generation. - Double-check the clean/dist-clean targets (or equivalent) to ensure they now remove the old per-flavor
$(BUILD).cssartifacts and correctly handle the singlestyle.cssfile.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting `style.css` into a Make variable (e.g. `STYLESHEET_NAME`) so the filename remains configurable and avoids scattering a hard-coded name across the build logic.
- If multiple flavors can be built concurrently, using a single `$(DEST_DIR)/style.css` target may introduce race conditions; it might be safer to either make the rule `.PHONY` or ensure flavor builds serialize the stylesheet generation.
- Double-check the clean/dist-clean targets (or equivalent) to ensure they now remove the old per-flavor `$(BUILD).css` artifacts and correctly handle the single `style.css` file.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
I believe this is now ready for review. |
Member
|
Based on what I can tell, this does seem like a change for the better. However, in the spirit of the Chesterton's Fence principle, before approving I'd also like to know: Why do we currently create a CCS file for each build? Was there a reason for this originally, and if so, what was that reason? |
Member
Author
|
#1002 (comment) suggests that if you run things in parallel then you can have problems, but I struggle to see how. Perhaps if you build multiple guides in parallel. Edit: perhaps it refers to the cache instead. |
aneta-petrova
approved these changes
Dec 5, 2025
Member
aneta-petrova
left a comment
aneta-petrova
left a comment
There was a problem hiding this comment.
Okay, thanks for the reply. I think it's worth a try.
evgeni
approved these changes
Dec 8, 2025
aneta-petrova
removed
Needs tech review
Member
|
It looks like shortly after this PR was merged, the GHA bot started posting unreasonably long lists of updated guides. For example: Or, in a PR that doesn't have impact on any guide at all: Does anyone have an idea of what could be causing this? Could it have something to do with the changes to the makefile that were introduced in this PR? |
Member
Author
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are you introducing?
Prior to this we created a CSS file for each build flavor, but with the same inputs. This duplication isn't needed so aligning on a single style.css leads to a quicker build and fewer files.
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
In #4506 we're introducing additional guides which makes the existing problem even bigger.
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Currently untested so I've made this a draft.Contributor checklists
Please cherry-pick my commits into:
Summary by Sourcery
Enhancements: