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.

Sign up for GitHub

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

ci: upload WASM and download in other workflows #147

Merged
merged 1 commit into from
Nov 7, 2024
Merged

ci: upload WASM and download in other workflows #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .github/workflows/release-plz.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ permissions:
contents: write

on:
push:
workflow_run:
workflows:
- prophet-wasmstan
types:
- "completed"
Comment on lines +8 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add conclusion: success to the workflow trigger.

The workflow should only run when the prophet-wasmstan workflow succeeds, not when it fails. This is especially important for a release workflow.

Apply this diff to ensure releases only happen after successful WASM builds:

  workflow_run:
    workflows:
      - prophet-wasmstan
    types:
      - "completed"
+    conclusion: success
    branches:
      - main
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
workflow_run:
workflows:
- prophet-wasmstan
types:
- "completed"
workflow_run:
workflows:
- prophet-wasmstan
types:
- "completed"
conclusion: success

branches:
- main

Expand Down
12 changes: 9 additions & 3 deletions .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
name: Rust

on:
push:
branches: [ main ]
pull_request:
workflow_run:
workflows:
- prophet-wasmstan
types:
- "completed"
Comment on lines +4 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Sequential workflow dependency is justified but can be optimized

The sequential dependency on prophet-wasmstan workflow is necessary but only for the test job. The current setup has these inefficiencies:

  • Only the test job requires the WASM artifact (prophet-wasmstan.wasm)
  • check, fmt, and clippy jobs are blocked unnecessarily

Recommend splitting this into two workflows:

  1. A parallel workflow for check, fmt, and clippy that runs on push/PR
  2. Keep the current sequential workflow only for test job that depends on prophet-wasmstan

This will optimize CI time while maintaining the required dependencies.

🔗 Analysis chain

Verify the sequential workflow dependency requirement.

The workflow now runs sequentially after prophet-wasmstan completes, which could increase overall CI time. Consider:

  1. Whether all jobs need the WASM artifact (currently only test job uses it)
  2. If jobs like check, fmt, and clippy could run in parallel with prophet-wasmstan
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the prophet-wasmstan workflow exists and its artifact usage

# Test 1: Check if the prophet-wasmstan workflow exists
echo "Verifying prophet-wasmstan workflow existence:"
fd -p ".github/workflows/*" -x grep -l "name:.*prophet-wasmstan"

# Test 2: Check which jobs/steps use the WASM artifact
echo -e "\nChecking WASM artifact usage in test commands:"
rg "prophet-wasmstan\.wasm" -A 3

Length of output: 313

Script:

#!/bin/bash
# Let's try a broader search to understand the workflow dependencies and WASM usage

# Check all workflow files
echo "=== All GitHub Workflow Files ==="
fd -p ".github/workflows/"

# Look for any WASM-related content in workflows
echo -e "\n=== WASM-related content in workflows ==="
rg -i "wasm" .github/workflows/

# Check for artifact download/usage in the rust.yml
echo -e "\n=== Artifact usage in rust.yml ==="
cat .github/workflows/rust.yml

# Look for any prophet-wasmstan references
echo -e "\n=== prophet-wasmstan references ==="
rg -i "prophet-wasmstan" .github/workflows/

Length of output: 3506


env:
CARGO_TERM_COLOR: always
Expand All @@ -26,6 +28,10 @@ jobs:
name: Tests
runs-on: ubuntu-latest
steps:
- uses: actions/download-artifact@v4
with:
name: prophet-wasmstan.wasm
path: crates/augurs-prophet/prophet-wasmstan.wasm
Comment on lines +31 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Move artifact download after checkout.

The artifact download step is positioned before the repository checkout, which could cause issues:

  1. The target directory structure doesn't exist yet
  2. The downloaded artifact might be affected by the subsequent checkout

Apply this change to fix the ordering:

-      - uses: actions/download-artifact@v4
-        with:
-          name: prophet-wasmstan.wasm
-          path: crates/augurs-prophet/prophet-wasmstan.wasm
      - name: Checkout sources
        uses: actions/checkout@v4

      - name: Install Rust toolchain
        uses: moonrepo/setup-rust@v1
+      - name: Download WASM artifact
+        uses: actions/download-artifact@v4
+        with:
+          name: prophet-wasmstan.wasm
+          path: crates/augurs-prophet/prophet-wasmstan.wasm
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- uses: actions/download-artifact@v4
with:
name: prophet-wasmstan.wasm
path: crates/augurs-prophet/prophet-wasmstan.wasm
- name: Checkout sources
uses: actions/checkout@v4
- name: Install Rust toolchain
uses: moonrepo/setup-rust@v1
- name: Download WASM artifact
uses: actions/download-artifact@v4
with:
name: prophet-wasmstan.wasm
path: crates/augurs-prophet/prophet-wasmstan.wasm

- name: Checkout sources
uses: actions/checkout@v4

Expand Down
5 changes: 4 additions & 1 deletion .github/workflows/wasmstan.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ on:
push:
branches: [ "main" ]
pull_request:
branches: [ "main" ]

env:
CARGO_TERM_COLOR: always
Expand All @@ -30,3 +29,7 @@ jobs:
- uses: actions/setup-node@v4
- name: Run node test
run: just components/test
- uses: actions/upload-artifact@v4
with:
name: prophet-wasmstan.wasm
path: components/cpp/prophet-wasmstan.wasm
Comment on lines +32 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

The artifact path in the workflow needs to be updated

The current path components/cpp/prophet-wasmstan.wasm in the workflow is incorrect. According to the build configuration in components/justfile, the WASM artifact is generated as prophet-wasmstan-component.wasm in the cpp/prophet-wasmstan directory. The correct path should be:

path: components/cpp/prophet-wasmstan/prophet-wasmstan-component.wasm
  • The build process generates prophet-wasmstan-component.wasm as shown in the build target of the justfile
  • The README also confirms this is the expected output filename
  • The rust.yml workflow attempts to download this artifact with a different name, indicating a mismatch in the artifact configuration
🔗 Analysis chain

LGTM! Verify the artifact path exists.

The artifact upload step is correctly configured using the latest version of the upload-artifact action.

Let's verify the artifact path exists in the repository:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the WASM artifact path exists and is generated during the build
# Expected: Find references to the WASM file in build scripts or configuration

# Check for references to the WASM file path in build scripts
rg -l "prophet-wasmstan\.wasm" components/

# Check if there are any build/compilation steps that generate this file
fd -t f "just.*" components/ -x grep -l "prophet-wasmstan\.wasm" {} \;

Length of output: 120

Script:

#!/bin/bash
# Let's try a broader search to understand the WASM build setup

# Check for any WASM-related build configurations
rg -l "\.wasm" components/cpp/

# Look for build scripts or configuration files in cpp component
fd -t f "(CMakeLists\.txt|Makefile|\.cmake|build\.rs|\.toml)" components/cpp/

# Check for any justfile in the repository
fd justfile

# Look for workflow files that might reference this artifact
rg -l "prophet-wasmstan" .github/workflows/

Length of output: 333

Script:

#!/bin/bash
# Let's check the content of relevant files to understand the build process

# Check the README for WASM build instructions
cat components/cpp/prophet-wasmstan/README.md

# Check the justfiles for WASM build targets
cat components/justfile
cat justfile

# Check other workflows that reference this component
cat .github/workflows/rust.yml

Length of output: 8743

Loading