Skip to content

fix(ci): install deps once then cache #1280

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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

neriumpete
Copy link

@neriumpete neriumpete commented Mar 9, 2025

Overview
This PR refactors the CI workflow to address the issue of redundant dependency installation and p2pd builds across multiple matrix jobs. By moving these steps into a dedicated job, we now install and cache dependencies only once per CI run, reducing overall build time and resource consumption.

Changes
Separated Dependency Setup:

  • Created a new job (setup-dependencies) that runs before the matrix-based test job.
  • Moved dependency installation steps—including setting up Nim, Go, Python, installing Nimble dependencies, and building the p2pd daemon—into this job.

Caching Improvements:
Nimble Dependencies: Caches the nimbledeps directory using a key based on Nim version, builder, CPU type, and a hash of the .pinned file.
p2pd Build Artifact: Implements caching for the p2pd build, so it’s only built if the cache isn’t found.

Enhanced Environment Consistency:
Both the setup and test jobs now consistently configure the environment (using the same custom actions for Nim, and the appropriate setup for Go and Python).
Improved directory creation by using mkdir -p to ensure robustness.

Concurrency Management:
The concurrency settings ensure that the setup job runs only once per branch or pull request, avoiding simultaneous redundant executions.

Impact

  • Reduced Redundancy: By isolating dependency setup and caching the results, each matrix test job restores pre-installed dependencies rather than re-installing them.
  • Faster CI Builds: This separation leads to more efficient use of resources and decreased overall build times.
    Improved Consistency: Ensures that all jobs run with the same pre-installed environment, reducing potential configuration discrepancies.
  • This change directly addresses the issue of multiple redundant dependency installations and p2pd builds by moving them to a dedicated, cached setup job.

Closes #1154

Copy link
Member

@vladopajic vladopajic left a comment

Choose a reason for hiding this comment

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

thanks for showing initiative.

instead of caching dependencies, it might be even better to optimize them by avoiding them.

  • building p2pd: could be downloaded as binary and added to path. this would avoid:
    • setup of go
    • downloading sources and building go-libp2p-daemon
  • can pyton be avoided?
  • could all platforms reuse nim dependencies? they are just source files? could we download them once and just reuse them in test jobs
  • could there be only one setup-dependencies job (platform impended) that is reused by test jobs? if so this way to go, if not then there is not need of splitting setup-dependencies and test because it will just introduce unnecessary blocking.

@github-project-automation github-project-automation bot moved this from new to In Progress in nim-libp2p Mar 10, 2025
@neriumpete neriumpete force-pushed the master branch 3 times, most recently from 1c99074 to a41f101 Compare March 10, 2025 19:25
@neriumpete
Copy link
Author

Note - I removed the i386 platform from the matrix because no official 32-bit p2pd binary is available, causing build failures. So it needs to be removed if we don't want to install go

Copy link
Member

@vladopajic vladopajic left a comment

Choose a reason for hiding this comment

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

@neriumpete sir, thanks for effort!

ci i currently failing, it looks like deamon is not in path or something? may i suggest to run changes in ci of your fork and push back here once it passes over there?

Copy link
Member

@vladopajic vladopajic left a comment

Choose a reason for hiding this comment

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

LGTM - if ci is passing.

@neriumpete it would be also nice to update PR title and description to match what has been done in PR since it was changed since initial commit.

@richard-ramos it will require approval from you, and also removing i386 from ci test requirements since it was removed.

@richard-ramos
Copy link
Member

I think it should be fine to drop Linux i386 since this architecture should be rare (looking for examples across internet it seems to be used on older machines). Let's cc: @arnetheduck for his blessings / insight.

--
Having said that, I see that other targets are failing with:

    Unhandled error: No such file or directory
Additional info: Could not find command: '/home/runner/work/nim-libp2p/nim-libp2p/.ci-bin/p2pd'. OS error: Exec format error [OSError]

Also, the commits must be signed.

@neriumpete neriumpete force-pushed the master branch 14 times, most recently from 09557d6 to d084bf5 Compare March 13, 2025 09:27
@neriumpete neriumpete marked this pull request as draft March 13, 2025 15:50
@richard-ramos
Copy link
Member

Yeah after giving it some thought, let's drop i386 support.

Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

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

LGTM!
Good work


# Verify p2pd before running tests
export PATH="${{ github.workspace }}/p2pdCache:$PATH"
echo "PATH: $PATH"
Copy link
Member

Choose a reason for hiding this comment

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

this echo is likely unneeded.

# Set GCC-14 as the default
sudo update-alternatives --set gcc /usr/bin/gcc-14

- name: Run tests
run: |
source .venv/bin/activate
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer needed. I ended up removing python from mbedtls (and ended up remobing mbedtls too :) )

Comment on lines +151 to +154
- name: Setup python
run: |
mkdir -p .venv
python -m venv .venv
Copy link
Member

@richard-ramos richard-ramos Mar 26, 2025

Choose a reason for hiding this comment

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

This can be removed

Suggested change
- name: Setup python
run: |
mkdir -p .venv
python -m venv .venv

Copy link
Member

Choose a reason for hiding this comment

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

@richard-ramos not what suggestion implies? did you forget to make empty suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

yeah... forgot to actually remove the code from my suggestion. fixed

Comment on lines +178 to +180
# Verify p2pd before running tests
export PATH="${{ github.workspace }}/p2pdCache:$PATH"
echo "PATH: $PATH"
Copy link
Member

Choose a reason for hiding this comment

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

there is # Verify it's working step above? is it really necessary to verify two times?

@@ -0,0 +1,57 @@
name: "Install p2pd"
Copy link
Member

Choose a reason for hiding this comment

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

this action didn't work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

CI: Install deps once and then cache
3 participants