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

CI Fixes #51

Merged
merged 77 commits into from
May 13, 2024
Merged

CI Fixes #51

merged 77 commits into from
May 13, 2024

Conversation

l-monninger
Copy link
Contributor

@l-monninger l-monninger commented May 10, 2024

  • Fixes CI.
  • Shares build context for check, just m1-da-light-node, just monza-full-node`.
  • Adds matrix for Macos and x86_64 Ubuntu.

@l-monninger l-monninger marked this pull request as draft May 10, 2024 18:54
@l-monninger
Copy link
Contributor Author

l-monninger commented May 12, 2024

https://github.com/movementlabsxyz/movement/actions/runs/9053736152/job/24872803247

This works. The last thing to try and fix for this PR would be the Monza derivation in a separate file. For some reason that build didn't work.

@l-monninger
Copy link
Contributor Author

Until we have Cachix or similar, one workflow will be better because it will share the build context.

@0xmovses
Copy link
Collaborator

@radupopa369 you used cachix before to get fast CI with nix? If I remember correctly.

@l-monninger l-monninger marked this pull request as ready for review May 12, 2024 20:32
@0xmovses
Copy link
Collaborator

I'll wait for the workflow to complete before I review! 🚀

@0xmovses
Copy link
Collaborator

From CI

Run sudo nix-collect-garbage -d
sudo: nix-collect-garbage: command not found

@l-monninger
Copy link
Contributor Author

For whatever reason the DeterminateSystems action doesn't install garbage collect which would help clean our environment. Anyways, just trying to add a few things. I'm going to pause here, so if the lates passes that'll be all for this PR, otherwise, I'll revert to f13c370

@l-monninger
Copy link
Contributor Author

@0xmovses @mzabaluev Pausing here for now: 0134626


steps:
- name: Checkout repository
uses: actions/checkout@v3
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is v4 now.

flake.nix Outdated

shellHook = ''
#!/bin/bash
export MONZA_APTOS_PATH=$(nix path-info -r .#monza-aptos | tail -n 1)
export PATH=$PATH:''${CARGO_HOME:-~/.cargo}/bin
export PATH=$PATH:''${RUSTUP_HOME:-~/.rustup}/toolchains/$RUSTC_VERSION-x86_64-unknown-linux-gnu/bin/
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. rustup is used to install the toolchain, but cannot be invoked in the shell to run the binaries?
  2. This is Linux-specific, I assume it's OK since it only runs in Nix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's see what happens if I drop either of these out.

@@ -1,4 +1,5 @@
#!/bin/bash -e
APP_PATH="./.etc/celestia/${CELESTIA_CHAIN_ID}/.celestia-app"
NODE_PATH="./.etc/celestia/${CELESTIA_CHAIN_ID}/bridge"
exec > /dev/null 2>&1 # TODO: drop this line once we have debugged
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-enable the output before it's merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah

done
echo "Discovered genesis: $GENESIS"

if [ "${#GENESIS}" -le 4 ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

The condition looks weird: why does genesis hash need to be at least 4 characters long, rather than just non-empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"null"


export CELESTIA_CUSTOM=$CELESTIA_CHAIN_ID:$GENESIS
echo "$CELESTIA_CUSTOM"

exec > /dev/null 2>&1 # TODO: drop when we have debugged
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the temporary hack here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIll do

@sre-movement
Copy link

@radupopa369 you used cachix before to get fast CI with nix? If I remember correctly.

yes @0xmovses used hosted solution from https://www.cachix.org/
I was not the one who made the setup tough :)

Copy link
Collaborator

@0xmovses 0xmovses left a comment

Choose a reason for hiding this comment

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

We will iterate in CI build and test speed as we go. Let's try Cachix. From my POV, this is good to merge!

@l-monninger
Copy link
Contributor Author

Goodbye broken window...

@l-monninger l-monninger merged commit febcd52 into main May 13, 2024
2 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.

None yet

4 participants