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

Ship mathjax and jquery #423

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Ship mathjax and jquery #423

merged 4 commits into from
Oct 11, 2024

Conversation

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks right on - @BrayanDSO --> thanks for the pointers on where/how to expose this stuff, looks like it was a big help

@mikehardy
Copy link
Member

If @BrayanDSO likes this then we should merge and publish

Note that ubuntu-24 is a new runner image for us (it just promoted to ubuntu-latest yesterday) so I expect that the "build quick" CI check on ubuntu may fail. That is not related to this PR and should not block it. See #422

@mikehardy
Copy link
Member

I will unblock the codeql and build-quick:ubuntu CI runs by rebasing after #424 merges

But macos and windows should not fail, there appears to be an actual problem with the main.rs changes


Diff in /Users/runner/work/Anki-Android-Backend/Anki-Android-Backend/build_rust/src/main.rs at line 102:
         artifacts_dir.join("web/reviewer.css"),
     )?;
 
-    copy_dir_all("anki/out/qt/_aqt/data/web/js/vendor/mathjax", artifacts_dir.join("web/vendor/mathjax"))?;
+    copy_dir_all(
+        "anki/out/qt/_aqt/data/web/js/vendor/mathjax",
+        artifacts_dir.join("web/vendor/mathjax"),
+    )?;
     copy_file(
-            "anki/out/ts/mathjax/mathjax.js",
-            artifacts_dir.join("web/mathjax.js"),
-        )?;
+        "anki/out/ts/mathjax/mathjax.js",
+        artifacts_dir.join("web/mathjax.js"),
+    )?;
     copy_file(
         "anki/out/node_modules/jquery/dist/jquery.min.js",
         artifacts_dir.join("web/jquery.min.js"),

Did this build locally for you @MorenoTropical ?

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

style looks fine as a start (though may need to expose mathjax.css as well if it is in some specific location), but has an actual build failure

also needs a rebase to unblock CI once #424 is in

@mikehardy
Copy link
Member

mikehardy commented Oct 11, 2024

⚠️ I just re-pushed to unblock CI with #424 merged - it was after your push just now so should not have clobbered anything, we'll see how CI goes

Looks like you pushed twice though 😬 please triple-check, apologies

@mikehardy
Copy link
Member

Okay, ubuntu seems to be going through now after pinning to ubuntu-22 and you're last push

WIndows seems to have a platform-specific issue:

Failed to execute: rsync --relative a11y/assisti.... etc

Two quick ideas:

  • rsync not available by default on windows, I need to check runner image included software and see, install if not present
  • platform-specific exec of rsync is failing (maybe needs to be "rsync.exe", maybe needs different directory seperators like \ instead of /

Some of the build stuff needed alteration prior because of platform-specific items, this may be another case of that, as these targets may never have been built on windows

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Anki references the files with a slightly different directory structure:

<link rel="stylesheet" type="text/css" href="http://127.0.0.1:7981/_anki/css/reviewer.css">
...
<script src="http://127.0.0.1:7981/_anki/js/mathjax.js"></script>
<script src="http://127.0.0.1:7981/_anki/js/vendor/mathjax/tex-chtml-full.js"></script>

So while Anki has a css/ and js/ folder, we ship everything under web/.

web/ was used originally for the Svelte pages, which were under qt/_aqt/data/web back then, so the name made sense.

As the Svelte pages are now shipped under sveltekit/, web/ is mostly a legacy name.

To avoid some mental juggling about the directories names, I suggest to mimic Anki's directory tree structure.

Since this is just a refactor, that's not essential for approving the PR and doesn't need to be done right now.

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Failed to execute: rsync --relative a11y/assistive-mml.js a11y/complexity.js a11y/explorer.js a11y/semantic-enrich.js output/chtml/fonts/woff-v2/MathJax_AMS-Regular.woff output/chtml/fonts/woff-v2/MathJax_Calligraphic-Bold.woff output/chtml/fonts/woff-v2/MathJax_Calligraphic-Regular.woff output/chtml/fonts/woff-v2/MathJax_Fraktur-Bold.woff output/chtml/fonts/woff-v2/MathJax_Fraktur-Regular.woff output/chtml/fonts/woff-v2/MathJax_Main-Bold.woff output/chtml/fonts/woff-v2/MathJax_Main-Italic.woff output/chtml/fonts/woff-v2/MathJax_Main-Regular.woff output/chtml/fonts/woff-v2/MathJax_Math-BoldItalic.woff output/chtml/fonts/woff-v2/MathJax_Math-Italic.woff output/chtml/fonts/woff-v2/MathJax_Math-Regular.woff output/chtml/fonts/woff-v2/MathJax_SansSerif-Bold.woff output/chtml/fonts/woff-v2/MathJax_SansSerif-Italic.woff output/chtml/fonts/woff-v2/MathJax_SansSerif-Regular.woff output/chtml/fonts/woff-v2/MathJax_Script-Regular.woff output/chtml/fonts/woff-v2/MathJax_Size1-Regular.woff output/chtml/fonts/woff-v2/MathJax_Size2-Regular.woff output/chtml/fonts/woff-v2/MathJax_Size3-Regular.woff output/chtml/fonts/woff-v2/MathJax_Size4-Regular.woff output/chtml/fonts/woff-v2/MathJax_Typewriter-Regular.woff output/chtml/fonts/woff-v2/MathJax_Vector-Bold.woff output/chtml/fonts/woff-v2/MathJax_Vector-Regular.woff output/chtml/fonts/woff-v2/MathJax_Zero.woff tex-chtml-full.js sre/mathmaps/de.json sre/mathmaps/en.json sre/mathmaps/es.json sre/mathmaps/fr.json sre/mathmaps/hi.json sre/mathmaps/it.json sre/mathmaps/nemeth.json /D/a/Anki-Android-Backend/Anki-Android-Backend/anki/out/qt/_aqt/data/web/js/vendor/mathjax

I managed to build it in my Windows machine. My rsync is from msys2, like README suggests. Can't tell why this is happening, but I'm gonna guess that's a \ / issue as well

@MorenoTropical
Copy link
Contributor Author

To avoid some mental juggling about the directories names, I suggest to mimic Anki's directory tree structure.

Done

@mikehardy
Copy link
Member

I have a new windows dev env that I never properly set up so you beat me to replicating the windows build failure, I'm just now done getting all the prerequisites installed and building

The readme does specify to use msys2 / pacman to install rsync and I think that's likely the cause - I believe the rsync there can handle either path separator (or, perhaps just expects the unix-style path separator) vs whatever rsync is being used (or maybe even not being found) currently.

So likely fix will be twiddling the workflow to also install the correct rsync etc

I see msys2 is installed on windows images (https://github.com/actions/runner-images/blob/main/images/windows/Windows2022-Readme.md#msys2) but it isn't on path, so it seems reasonable to assume there is no way the rsync from msys2 could be on the path

I'll fix that up

@mikehardy
Copy link
Member

mikehardy commented Oct 11, 2024

just added the commit that should get windows working in CI ? 🤞
[edit: nope, looks like that blew up immediately, so I'll be pushing that commit again to hopefully make it work...]

The path changes look good to me - I'm sorry to ping you Brayan, I don't do it lightly - but I also know you have strong + good opinions in this specific area so with the paths changed now, I think it's good for you to have a look before they're shipped with new paths...

Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

Nice! Thanks

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

everything appears to be working well no on windows ✅
everything else has been thoroughly reviewed ✅

@mikehardy mikehardy added the pending-merge Waiting on CI or question responses to merge, but otherwise ready label Oct 11, 2024
@mikehardy mikehardy merged commit 0bf326d into ankidroid:main Oct 11, 2024
6 checks passed
@mikehardy mikehardy removed the pending-merge Waiting on CI or question responses to merge, but otherwise ready label Oct 11, 2024
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.

Get MathJax and jquery directly from the backend instead of copying it with a GitHub workflow
3 participants