Skip to content

Fix build:all output on Windows #8089

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 3 commits into from
Mar 14, 2025

Conversation

kamilkrzyskow
Copy link
Collaborator

@kamilkrzyskow kamilkrzyskow commented Mar 14, 2025

Hello 👋,
so this isn't a new bug, it's always been like that 👀 💦 Previously, I typically made changes to Python files, so I changed them inside of material directory and later copied them over to src, and I was caught red handed when working on the info plugin when I didn't update the build logic to not delete a custom .gitignore file😅

When following the development guide for the theme it mentions to build the project with build:all in preparation for a PR, as this will recreate all files and make sure everything is updated.

The issue is that the logic relied on the placement of / separators in strings, and Windows uses \, so none of the plain assets/javascripts/bundle.js paths got converted back to bundle.1a2b3c.js for the deployment.

When running the npm run build:all command with any commit before this PR on Windows and without doing any changes this would be the output of git status:

$ git status
HEAD detached at 3296cdf20
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   docs/schema/assets/icons.json
        modified:   material/overrides/assets/javascripts/iconsearch_index.json
        modified:   material/overrides/assets/stylesheets/custom.7c86dd97.min.css.map
        modified:   material/overrides/main.html
        modified:   material/templates/.icons/material/LICENSE
        modified:   material/templates/assets/stylesheets/main.4af4bdda.min.css.map
        modified:   material/templates/assets/stylesheets/palette.06af60db.min.css.map
        modified:   material/templates/base.html

Most of these are files where the paths weren't replaced because they used the \ separator, when on Windows.

After fixing the first issue the next bubbled up:

--- a/docs/schema/assets/icons.json
+++ b/docs/schema/assets/icons.json
@@ -11439,8 +11439,8 @@
     "simple/hiltonhotelsandresorts",
     "simple/hitachi",
     "simple/hive",
-    "simple/hive_blockchain",
     "simple/hivemq",
+    "simple/hive_blockchain",
     "simple/homarr",
     "simple/homeadvisor",
     "simple/homeassistant",
@@ -12829,9 +12829,9 @@
     "simple/spreadshirt",
     "simple/spreaker",
     "simple/spring",
-    "simple/spring_creators",
     "simple/springboot",
     "simple/springsecurity",
+    "simple/spring_creators",

The icons are gathered like all other paths, using the resolve function which in turn uses the tiny-glob npm module. Based on this issue my observation is that depending on the OS the order of files differs when there is _ in the file name.

While fixing this I've noticed that the icon search index doesn't contain the latest emojis. Not sure how this happened 🤔
Only explanation I have is that somehow you haven't updated the twemoji_db file on your local system for the last 6 months. You can see woman_walking_facing_right is not found in the deployment and it's part of the twemoji_db

The last thing which could be fixed to declutter the git status noise, is the .icons/material/LICENSE file which contains the CRLF line endings and this file is marked as modified, despite being converted to LF later. But I left it as-is.

Checked on MX Linux VM, and it does still work on the UNIX system, so I think macOS should work as well. Also all changes which would decrease performance (so everything) I tried to limit to Windows only, therefore if there is no need for sorting or path separator replacement, the other OS just use the data like before.

@squidfunk
Copy link
Owner

squidfunk commented Mar 14, 2025

Thanks for the PR, and for solving a long-standing issue 🚀 I can confirm that npm run build:all and npm start still work as expected on macOS. I can't currently commit to maintain the code in this PR, as testing on Windows is cumbersome for me (don't have a machine right at my disposal). Would you feel comfortable maintaining it?

I've also updated twemoji_db.py on my machine. I agree that the process that we currently have, building some stuff locally, is not optimal, and I can assure that we will all fix it as part of the foundational work we're on. Not everything at once, but we're getting there 🤟

@kamilkrzyskow
Copy link
Collaborator Author

I can confirm that npm run build:all and npm start still work as expected on macOS

Thanks for checking.

Would you feel comfortable maintaining it?

I'm not sure what maintaining it entails. if you're asking if I can be pinged to fix another Windows build issue, then sure, I'll take a look and "fix" the issue at hand. The definition of "fix" could change in the future, and I might be forced to remove the Windows scope of the current fixes and I would then replace them with a general fix like e.g. using localeCompare to change sorting for all OS, decreasing the build performance for all developers.

If maintaining it entails, coming back periodically and checking if the build on Windows still works as expected, then I guess a GitHub workflow with automatic tests would be better IMHO, as it would show a de-sync immediately. For now I think it's not a requirement, as the build logic doesn't change frequently.

I did a force push with a small fix to the alphabet and improved naming. So it's ready for merge ✌️

@squidfunk
Copy link
Owner

Perfect, thanks for checking!

@squidfunk squidfunk merged commit 0918657 into squidfunk:master Mar 14, 2025
4 checks 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.

2 participants