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

Migrate docs build system from pnpm to bun #1268

Merged

Conversation

SchahinRohani
Copy link
Contributor

@SchahinRohani SchahinRohani commented Aug 19, 2024

Description

  • Migrate the docs build system from pnpm to bun
  • Set up GitHub workflow with Deno Deploy
  • Configure Biome for code quality
  • Remove unnecessary packages (Playform Compression causes issues)
  • Refactore some of the Docs code for Code Quality.
  • Update documentation to reflect new setup
  • Add necessary dependencies in flake.nix

Fixes #1082

Preview Deploy NativeLink Docs Github Action Workflow
Preview the deployed NativeLink Docs

Type of change

Please delete options that aren't relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

@SchahinRohani SchahinRohani force-pushed the update/docs/build-system branch 10 times, most recently from 822e79c to 2dbf9f9 Compare August 21, 2024 11:28
Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

+@blakehatch
+@aaronmondal

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 25 files reviewed, and pending CI: Cargo Dev / macos-13 (waiting on @aaronmondal and @blakehatch)

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Fantastic change! This finally gives us full control over the entire website deployment process. Also it's way nicer to work with bun and the deno deployment is even faster than cloudflare 🚀

Reviewed 17 of 24 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and pending CI: Cargo Dev / macos-13, and 17 discussions need to be resolved (waiting on @aaronmondal and @blakehatch)


-- commits line 2 at r2:
Add "Fixes #1082" to the commit message since it fixes that issue.


flake.nix line 429 at r2 (raw file):

              bazel

              ## Infrastructure Stack

flake.nix line 447 at r2 (raw file):

              pkgs.kustomize

              ## Web Stack

flake.nix line 451 at r2 (raw file):

              pkgs.deno
              pkgs.lychee
              pkgs.nodejs_22 # Only for Pagefind.

Try whether we can get away with pkgs.nodejs-slim_22 which has a smaller closure size.


.github/workflows/native-docs.yaml line 14 at r2 (raw file):

  deploy:
    name: Docs Deployment
    runs-on:  ubuntu-22.04

Use Ubuntu 24.04 since it might be slightly faster.


.github/workflows/native-docs.yaml line 29 at r2 (raw file):

        run: |
          nix develop --impure --command bash -c "
            deno install -Arf jsr:@deno/deployctl &&

Pin the @deno/deployctl to a hashlocked version.


.github/workflows/native-docs.yaml line 34 at r2 (raw file):

          "

      - name: Cache Nix derivations

This step should come before the deploy step.


docs/.gitignore line 34 at r2 (raw file):

stats.html

# Deno Deploy File

Suggestion:

file

docs/biome.json line 10 at r2 (raw file):

    "enabled": true,
    "rules": {
      "recommended": true,

Could we put this back to "all" and disable only the warnings that really don't make sense (like the naming convention warning because of the generated md_to_mdx and metaphase types)? I'd like this to be as strict as possible.


docs/biome.json line 17 at r2 (raw file):

      }
    },
    "ignore": [".astro", "dist", "node_modules", ".wrangler"]

I think it's fine to remove the .wrangler here. It does break previous users once, but they should remove the old .wrangler directory anyways.


docs/package.json line 17 at r2 (raw file):

    "fix": "bun lint && bun format && bun sync",
    "format": "biome format --write .",
    "lint": "biome check --write --unsafe .",

Linting should only print linter warnings, not fix them. The format script should instead do the file transformations.


docs/package.json line 21 at r2 (raw file):

    "serve": "deno run --allow-net --allow-read --allow-env ./dist/server/entry.mjs",
    "deploy": "deployctl deploy --include=./dist --entrypoint=./dist/server/entry.mjs --save-config",
    "prod": "bun install && bun docs && bun run build && bun run deploy"

I believe this install needs to use --frozen-lockfile to prevent bun from autoupdating the bun.lockb.


docs/package.json line 48 at r2 (raw file):

  },
  "overrides": {
    "playwright-core": "1.44.0"

Is 1.44.0 correct or do we have to use 1.44.1 like previrously?


docs/README.md line 71 at r2 (raw file):

## 🐛 Known issues

- `bun metaphase` isn't working with the nix version of Bazel,

This is only true on MacOS. Linux doesn't require this.


tools/nixpkgs_bun.diff line 29 at r2 (raw file):

         url = "https://github.com/oven-sh/bun/releases/download/bun-v${version}/bun-darwin-x64-baseline.zip";
-        hash = "sha256-5PLk8q3di5TW8HUfo7P3xrPWLhleAiSv9jp2XeL47Kk=";
+        hash = "sha256-nf75MyU6ye7xHwdxJpaAGcg4MbJ7v2OUgEbepaUE5kg=";

This seems to be the wrong hash: https://github.com/TraceMachina/nativelink/actions/runs/10488998422/job/29052651314?pr=1268#step:6:286


docs/src/styles/custom.css line 19 at r2 (raw file):

html {
  scroll-behavior: smooth;

@blakehatch Do we want this globally?


docs/src/utils/metaphase_aot.ts line 1 at r2 (raw file):

import { join } from "node:path";

Hmm are we sure that biome properly captures this file? This should trigger a node module warning that has to be explicitly ignored.

@aaronmondal
Copy link
Member

@blakehatch I believe we need to adjust the github settings so that deno deploy can deploy from this repo.

We'll also need to run some tests to ensure that this works properly on PRs. @SchahinRohani We probably need to adjust the workflow with a conditional so that it only builds and lints the docs on PRs but runs the entire deploy step on pushes to main. We can do this by splitting the deno invocations in two steps and using an if like here:

if: github.ref == 'refs/heads/main'

@SchahinRohani SchahinRohani force-pushed the update/docs/build-system branch 6 times, most recently from fa34b4f to 7aa81c0 Compare August 22, 2024 10:34
Copy link
Contributor Author

@SchahinRohani SchahinRohani left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained, and 15 of 26 files reviewed, and pending CI: Cargo Dev / macos-13, Docs Deployment, Installation / macos-13, Installation / macos-14, Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (22.04), and 17 discussions need to be resolved (waiting on @aaronmondal and @blakehatch)


-- commits line 2 at r2:

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Add "Fixes #1082" to the commit message since it fixes that issue.

done.


flake.nix line 429 at r2 (raw file):

              bazel

              ## Infrastructure Stack

done.


flake.nix line 447 at r2 (raw file):

              pkgs.kustomize

              ## Web Stack

done.


flake.nix line 451 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Try whether we can get away with pkgs.nodejs-slim_22 which has a smaller closure size.

Looks like we don't even need nodejs for now. It builds on my test/workflow without having nodejs.
Since the Nix Installation on MacOS is failing on #1270 i dont think its a problem with the flake dependencies.


.github/workflows/native-docs.yaml line 14 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Use Ubuntu 24.04 since it might be slightly faster.

ok.


.github/workflows/native-docs.yaml line 29 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Pin the @deno/deployctl to a hashlocked version.

ok.


.github/workflows/native-docs.yaml line 34 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This step should come before the deploy step.

ok.


docs/.gitignore line 34 at r2 (raw file):

stats.html

# Deno Deploy File

ok.


docs/biome.json line 10 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Could we put this back to "all" and disable only the warnings that really don't make sense (like the naming convention warning because of the generated md_to_mdx and metaphase types)? I'd like this to be as strict as possible.

ok.


docs/biome.json line 17 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I think it's fine to remove the .wrangler here. It does break previous users once, but they should remove the old .wrangler directory anyways.

ok.


docs/package.json line 17 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Linting should only print linter warnings, not fix them. The format script should instead do the file transformations.

I have also added a check.lint for only printing linter warnings. It is convenient to work with the bun lint command.


docs/package.json line 21 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I believe this install needs to use --frozen-lockfile to prevent bun from autoupdating the bun.lockb.

The bun.lockb should be frozen now, so the bun install should not make any difference to the lockfile.


docs/package.json line 48 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Is 1.44.0 correct or do we have to use 1.44.1 like previrously?

I used the version thats pinned in the nixpkgs_playwright.diff.


docs/README.md line 71 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This is only true on MacOS. Linux doesn't require this.

Rewrited.


tools/nixpkgs_bun.diff line 29 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This seems to be the wrong hash: https://github.com/TraceMachina/nativelink/actions/runs/10488998422/job/29052651314?pr=1268#step:6:286

fixed.


docs/src/styles/custom.css line 19 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

@blakehatch Do we want this globally?

Just have a look on the desktop version on https://nativelink-docs.deno.dev and use the right sidebar. You will see the effect and i would recommend it.


docs/src/utils/metaphase_aot.ts line 1 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Hmm are we sure that biome properly captures this file? This should trigger a node module warning that has to be explicitly ignored.

fixed

Copy link
Contributor Author

@SchahinRohani SchahinRohani left a comment

Choose a reason for hiding this comment

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

@aaronmondal I have implemented a test deployment on PR and a Production Deployment on push on main.

I believe that's a good strategy to preview test deployments before merging them into main.

@blakehatch To enable automatic authorization for deployments, please log in to https://dash.deno.com/login once. After that, I can invite you to the Nativelink organization, allowing you to manage the deployment of the Nativelink GitHub repository.

Reviewable status: 0 of 2 LGTMs obtained, and 15 of 26 files reviewed, and pending CI: Docs Deployment, Installation / macos-13, Installation / macos-14, Remote / large-ubuntu-22.04, and 17 discussions need to be resolved (waiting on @aaronmondal and @blakehatch)

Copy link
Contributor Author

@SchahinRohani SchahinRohani left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained, and 15 of 26 files reviewed, and pending CI: Docs Deployment, Installation / macos-13, Installation / macos-14, Remote / large-ubuntu-22.04, and 17 discussions need to be resolved (waiting on @aaronmondal and @blakehatch)


tools/nixpkgs_bun.diff line 29 at r2 (raw file):

Previously, SchahinRohani (Schahin) wrote…

fixed.

The sha256 of the bun package is the right version, so there was no fix fo this.

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r3, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and pending CI: Docs Deployment, Installation / macos-14, and 12 discussions need to be resolved (waiting on @blakehatch)


-- commits line 2 at r2:

Previously, SchahinRohani (Schahin) wrote…

done.

Hmm maybe bugged in reviewable? It's ok I can add this when merging.


.github/workflows/native-docs.yaml line 29 at r2 (raw file):

Previously, SchahinRohani (Schahin) wrote…

ok.

Is there a way to pin this to a hash instead of a version? Versions could be overridden which is a supply-chain security risk.


docs/README.md line 71 at r3 (raw file):

## 🐛 Known issues

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,

Suggestion:

 doesn't work

docs/README.md line 71 at r3 (raw file):

## 🐛 Known issues

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,

Suggestion:

.

docs/README.md line 71 at r3 (raw file):

## 🐛 Known issues

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,

Suggestion:

MacOS

docs/README.md line 72 at r3 (raw file):

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,
  The solution to this: Install Bun and Bazel on your host and build the docs outside the flake.

Suggestion:

As a workaround i

docs/README.md line 73 at r3 (raw file):

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,
  The solution to this: Install Bun and Bazel on your host and build the docs outside the flake.
- `bun dev` isn't reloading the changes in the starlight.conf.ts

Suggestion:

doesn't reload

tools/nixpkgs_bun.diff line 29 at r2 (raw file):

Previously, SchahinRohani (Schahin) wrote…

The sha256 of the bun package is the right version, so there was no fix fo this.

I think there is a duplicate == at the end now.


docs/src/utils/md_to_mdx.ts line 169 at r1 (raw file):

}

export function preProcessMarkdown(markdown: string): string {

No need to rename this.


docs/src/utils/md_to_mdx.ts line 230 at r3 (raw file):

}

function isSpecialLine(line: string): boolean {

This should probably be called isAside: https://starlight.astro.build/guides/components/#asides

Code quote:

SpecialLine

@SchahinRohani SchahinRohani force-pushed the update/docs/build-system branch 3 times, most recently from a7afde8 to 4e847c7 Compare August 23, 2024 08:17
Copy link
Contributor Author

@SchahinRohani SchahinRohani left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained, and 21 of 26 files reviewed, and pending CI: Analyze (javascript-typescript), Docs Deployment, Publish image, Publish nativelink-worker-init, asan / ubuntu-22.04, docker-compose-compiles-nativelink (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, and 12 discussions need to be resolved (waiting on @aaronmondal and @blakehatch)


-- commits line 2 at r2:

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Hmm maybe bugged in reviewable? It's ok I can add this when merging.

My bad! Now it should be in the commit message 😄


docs/README.md line 71 at r3 (raw file):

## 🐛 Known issues

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,

Done.


docs/README.md line 71 at r3 (raw file):

## 🐛 Known issues

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,

Done.


docs/README.md line 71 at r3 (raw file):

## 🐛 Known issues

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,

Done.


docs/README.md line 72 at r3 (raw file):

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,
  The solution to this: Install Bun and Bazel on your host and build the docs outside the flake.

Done.


docs/README.md line 73 at r3 (raw file):

- `bun run docs.build` isn't working on `MacOS` with the nix version of Bazel,
  The solution to this: Install Bun and Bazel on your host and build the docs outside the flake.
- `bun dev` isn't reloading the changes in the starlight.conf.ts

Done.


tools/nixpkgs_bun.diff line 29 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I think there is a duplicate == at the end now.

I think the sha256sum is valid for the bun package.


.github/workflows/native-docs.yaml line 29 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Is there a way to pin this to a hash instead of a version? Versions could be overridden which is a supply-chain security risk.

I moved it from the workflow in to the package.json.


docs/src/utils/md_to_mdx.ts line 169 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

No need to rename this.

Ok.


docs/src/utils/md_to_mdx.ts line 230 at r3 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This should probably be called isAside: https://starlight.astro.build/guides/components/#asides

This is getting more into the actual docs generation, which should not be a part of this PR. This is only a quick fix for the successful integration of Biome without any warnings and errors.

@SchahinRohani SchahinRohani mentioned this pull request Aug 23, 2024
4 tasks
@SchahinRohani SchahinRohani mentioned this pull request Aug 23, 2024
4 tasks
Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

:lgtm:

@blakehatch and I need to set up the repo for this before we can merge, but I think this is good now. Great job!

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Installation / macos-13, docker-compose-compiles-nativelink (22.04), integration-tests (22.04) (waiting on @blakehatch)

Copy link
Member

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

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

:LGTM: Everything functional for me and significantly faster cuz of Bun.

Reviewable status: :shipit: complete! 2 of 2 LGTMs obtained, and all files reviewed


docs/src/styles/custom.css line 19 at r2 (raw file):

Previously, SchahinRohani (Schahin) wrote…

Just have a look on the desktop version on https://nativelink-docs.deno.dev and use the right sidebar. You will see the effect and i would recommend it.

For docs I would like it globally.

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

:lgtm:

I'll merge as soon as we have the deno deploy app set up.

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: :shipit: complete! 2 of 2 LGTMs obtained, and all files reviewed

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 2 of 2 LGTMs obtained, and all files reviewed

@aaronmondal aaronmondal merged commit ef3a8a6 into TraceMachina:main Sep 4, 2024
30 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.

Add documentation tests to CI
4 participants