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

Github CI and Cargo.lock get out of sync #2620

Open
michael-kerscher opened this issue Feb 3, 2025 · 5 comments
Open

Github CI and Cargo.lock get out of sync #2620

michael-kerscher opened this issue Feb 3, 2025 · 5 comments

Comments

@michael-kerscher
Copy link
Collaborator

michael-kerscher commented Feb 3, 2025

The Github CI workflows use specific versions for mdbook tools that are different from what developers are using. E.g. mdbook is currently using 0.4.37:

run: cargo install mdbook --locked --version 0.4.37

but Cargo.lock would use 0.4.44 - this is regularly bumped via dependabot like in 010bd29

comprehensive-rust/Cargo.lock

Lines 1640 to 1641 in 4218c95

name = "mdbook"
version = "0.4.44"

and the developers are even instructed to use a completely different version that might not be one of those as it is not even locked (other tools are locked and might not be compatible with the "random" (most up-to-date) version of mdbook

cargo install mdbook
cargo install --locked mdbook-svgbob
cargo install --locked mdbook-i18n-helpers
cargo install --locked i18n-report
cargo install --locked mdbook-linkcheck
cargo install --locked --path mdbook-exerciser
cargo install --locked --path mdbook-course

We should use the same versions for developers and what the CI uses.

This is especially interesting for people developing javascript/theme code as mdbook generates/copies lots of files into the target folder if they are not overriden by us.

For example our book.js is copied but https://google.github.io/comprehensive-rust/css/general.css is not in our repository not existing general.css

Two options (second is my prefered proposal)

  • Keep manual overrides for the Github CI: Dev and CI are not in sync and might cause issues when someone changes the javascript code or relies on its functionality (when using mdbook serve - as in Incorrect behavior with mdbook serve compared to published version. #2588
  • Fix the instruction to install the --locked version of mdbook and use this version in the Github CI as well. Caveat: Dependabot updates need to be reviewed carefully and as we override some things (e.g. index.hbs) we need to carefully track updates to the official files with each update.
    • To make this update process reliable, write tests for the things we rely on in tests/src so things like missing theme buttons or search buttons are visible immediately.
@djmitche
Copy link
Collaborator

djmitche commented Feb 3, 2025

I think it's GitHub, not Gitlab :)

The second option seems good. We could also consider

  • Disable dependabot updates of mdbook and friends
  • Copy all files from the mdbook theme, so that nothing is supplied. This might at least break less often.

@michael-kerscher
Copy link
Collaborator Author

Yes, Github ^^ - I'm going to fix all occurrences above (and in the title)

Copying all files could lead to more reliable builds but could also fail in other spectacular ways and additionally comes with more update toil as we would need to regenerate everything on each update, otherwise we have an unknown combination of code.
I would like to try to keep updating toil at a minimum.

@michael-kerscher michael-kerscher changed the title Gitlab CI and Cargo.lock get out of sync Github CI and Cargo.lock get out of sync Feb 3, 2025
michael-kerscher added a commit that referenced this issue Feb 4, 2025
Test the main page for existence of
- theme button visible
  - clicking shows the list of themes
- search button visible
  - and tests successful search for "Welcome"
- language button visible
  - clicking shows the list of languages

this is testing functionality missing in dev environments in
#2588 and is relevant
for #2620
@michael-kerscher
Copy link
Collaborator Author

The approach I describe above does not work that way. https://doc.rust-lang.org/cargo/commands/cargo-install.html#dealing-with-the-lockfile:

The --locked flag can be used to force Cargo to use the packaged Cargo.lock file if it is available.

While this is partly what we want, we don't specify versions that need to be installed. The Cargo.lock file in this repository is irrelevant in choosing the installed versions of mdbook and the mdbook plugins.

Another way forward can be an install script that is used by both the CI and the developers to install exactly the versions that we want and have tested. This will replace the manual versions in action.yml and can also replace the developer instructions

@michael-kerscher
Copy link
Collaborator Author

@mgeisler: While looking at this issue I noticed these warnings from e.g. https://github.com/google/comprehensive-rust/actions/runs/13132471845/job/36641626915 - the step "Building bn translation as of 2024-03-03T20:37:57+05:30"

  2025-02-04 10:11:46 [INFO] (mdbook::book): Book building has started
  Warning: The gettext preprocessor was built against mdbook version 0.4.40, but we're being called from version 0.4.37
  2025-02-04 10:11:47 [INFO] (mdbook::book): Running the exerciser backend
  2025-02-04 10:11:47 [INFO] (mdbook::renderer): Invoking the "exerciser" renderer
  Warning: The gettext preprocessor was built against mdbook version 0.4.40, but we're being called from version 0.4.37
  2025-02-04 10:11:47 [INFO] (mdbook::book): Running the html backend
  Warning: The gettext preprocessor was built against mdbook version 0.4.40, but we're being called from version 0.4.37
  2025-02-04 10:11:49 [INFO] (mdbook::book): Running the linkcheck backend
  2025-02-04 10:11:49 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
  2025-02-04 10:11:49 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.
  Warning: The gettext preprocessor was built against mdbook version 0.4.40, but we're being called from version 0.4.37

We need to find a good and reliable way to sync the versions of all required versions.

@michael-kerscher
Copy link
Collaborator Author

michael-kerscher commented Feb 5, 2025

Regarding the comment above this one this is coming from mdbook-i18n-helpers//i18n-helpers/mdbook-gettext that has its own lockfile and mdbook version that is locks to mdbook-i18n-helpers/Cargo.lock (tag: 0.3.3) that is different (if we don't take any precaution) from the mdbook version installed in this repo (via install-mdbook.sh - see PR above)

Syncing the versions is currently done manually and in different places

michael-kerscher added a commit that referenced this issue Feb 5, 2025
… CI (#2626)

Move mdbook installation into a script and use exact versions from the
CI.
Update README.md to instruct developers to use the same versions as the
CI to sync both environments.
This is related to #2620 and it fixes #2588
michael-kerscher added a commit to michael-kerscher/comprehensive-rust that referenced this issue Feb 7, 2025
…#2621)

Test the main page for existence of
- theme button visible
  - clicking shows the list of themes
- search button visible
  - and tests successful search for "Welcome"
- language button visible
  - clicking shows the list of languages

this is testing functionality missing in dev environments in
google#2588 and is relevant
for google#2620
michael-kerscher added a commit to michael-kerscher/comprehensive-rust that referenced this issue Feb 7, 2025
… CI (google#2626)

Move mdbook installation into a script and use exact versions from the
CI.
Update README.md to instruct developers to use the same versions as the
CI to sync both environments.
This is related to google#2620 and it fixes google#2588
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 a pull request may close this issue.

2 participants