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

Update CI #552

Merged
merged 51 commits into from
Oct 3, 2024
Merged

Update CI #552

merged 51 commits into from
Oct 3, 2024

Conversation

b-yap
Copy link
Contributor

@b-yap b-yap commented Sep 17, 2024

To avoid disk space issues or similar to this:

The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

The hosted runner encountered an error while running your job. (Error Type: Disconnect).

Received request to deprovision: The request was cancelled by the remote provider.

I've split one huge job into multiple jobs (as suggested in actions/runner-images#10401)

New CI

The jobs have shared actions:

└── prerequisite-nightly
      ├── prerequisite
      │   ├── free-up-disk-space
      │   └── installations
      │        └── cache, protoc
      └── install-rust-nightly
          └── setup nightly
  • Jobs using stable rust version uses prerequisite action.
  • Jobs using nightly rust version uses prerequisite-nightly action.

Flow of the jobs:

└── Check Build (stable)
      ├── Run Clippy and Format Checks (stable)
      ├── Run Clippy and Format Checks (nightly)
      └-├─ Run Tests for Pallets (nightly)
      └-├─ Run Tests for Clients (nightly)
        └── Run Tests for Vault (nightly)

Cleanup Dependencies

Ran #![deny(unused_crate_dependencies)] on the pallets (and other directories) to identify the dependencies to:

  • remove; or
  • move to [dev-dependencies].

Most of these are serde(still questionable), sp-io, sp-runtime and sp-core, pallet-balances and pallet-timestamp to name a few.

All other touched files are caused by clippy.


# Substrate dependencies
sp-runtime = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: sp-runtime is a dependency found in frame-support. We don't need to add sp-runtime as a dependency.

sp-runtime = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
sp-arithmetic = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
sp-std = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
sp-io = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sp-core and sp-io are only used for testing. I've moved them to [dev-dependencies]

frame-support = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
frame-system = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
pallet-balances = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pallet-balances is used only in the tests; moving it to [dev-dependencies].

scripts/cmd-all Outdated
@@ -45,6 +45,7 @@ cargo $sub_cmd -p oracle --all-features $options
cargo $sub_cmd -p pooled-rewards --all-features $options
cargo $sub_cmd -p redeem --all-features $options
cargo $sub_cmd -p replace --all-features $options
cargo $sub_cmd -p reward --all-features $options
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we forgot to check reward

@@ -174,8 +177,7 @@ mod math {
.ok_or(ArithmeticError::Underflow)?
.checked_div(accuracy)
.ok_or(ArithmeticError::Underflow)?
.try_into()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a clippy suggestion

@@ -5,82 +5,64 @@ authors = ["Pendulum"]
edition = "2021"

[dependencies]
serde = { version = "1.0.130", default-features = false, features = ["derive"], optional = true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

serde was flagged as unused dependency by
#![deny(unused_crate_dependencies)]

@@ -245,8 +246,8 @@ impl<T: Config> Pallet<T> {
}

ext::vault_registry::pool_manager::withdraw_collateral::<T>(
&vault_id,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vault_id and nominator_idare both reference variables,&` is not needed anymore.

(From Clippy)


frame-support = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
frame-system = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
frame-benchmarking = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false, optional = true }
pallet-balances = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only used during testing

@@ -142,9 +142,7 @@ where
let Ok(coin_info) = Dia::get_coin_info(blockchain, symbol) else { return None };

let value = ConvertPrice::convert(coin_info.price)?;
let Some(timestamp) = ConvertMoment::convert(coin_info.last_update_timestamp) else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy suggestion. ? returns None for Options; Err for Result.

@@ -247,7 +247,7 @@ impl<T: Config> Pallet<T> {
}

let current_status_is_online = Self::is_oracle_online();
let new_status_is_online = oracle_keys.len() > 0 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy suggestion

@@ -39,15 +39,11 @@ pub mod benchmarks {
collateral_currency,
);

let _stake = T::VaultStaking::deposit_stake(&vault_id, &nominator, nominated_amount.into())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trait Staking in pallet-staking returns Result<(), DispatchError>; so no need for let <variable_name>.

sp-std = {git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false}



[dev-dependencies]
mocktopus = "0.8.0"
sp-io = {git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.1.0", default-features = false}
Copy link
Contributor Author

@b-yap b-yap Sep 20, 2024

Choose a reason for hiding this comment

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

we need the std feature, to avoid externalities issues, hence default-features should be true:

error: cannot find macro `thread_local` in this scope
  --> /home/runner/.cargo/git/checkouts/polkadot-sdk-67b6d29d759a1af6/e51a91f/substrate/primitives/externalities/src/scope_limited.rs:22:1
   |
22 | environmental::environmental!(ext: trait Externalities);
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `thread_local` is in scope, but it is an attribute: `#[thread_local]`
   = note: this error originates in the macro `$crate::thread_local_impl` which comes from the expansion of the macro `environmental::environmental` (in Nightly builds, run with -Z macro-backtrace for more info)

See failed CI

@@ -186,7 +186,7 @@ pub fn validate_envelopes<'a, T: Config>(
let (value, n_h) = get_externalized_info(envelope)?;

// Check if the tx_set_hash matches the one included in the envelope
let tx_set_hash = Pallet::<T>::get_tx_set_hash(&value)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

value here is already a reference; & is not needed.

(Clippy error)

@@ -23,69 +23,11 @@ echo "------------------------------------------------------------------------"

cargo $sub_cmd -p spacewalk-primitives --all-features $options

echo "------------------------------------------------------------------------"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Broken down this script into multiple ones, to avoid the issue aforementioned in the description.

@@ -479,7 +479,7 @@ impl XCMCurrencyConversion for SpacewalkNativeCurrencyKey {
if blockchain.len() != 1 && blockchain[0] != 0 || symbol.len() != 1 {
return None;
}
return Some(symbol[0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clippy suggestion

@b-yap b-yap marked this pull request as ready for review September 24, 2024 05:37
@b-yap b-yap requested a review from a team September 24, 2024 05:37
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Really nice job, it's amazing @b-yap 🚀 sorry for the delayed review!
Only left two comments but we can merge after

Comment on lines +27 to +33
./scripts/cmd-pallets "$reason" "$sub_cmd" "$options"

if [[ $sub_cmd == *"nightly"* ]]; then
cargo $sub_cmd -p currency $options
else
echo "------------------------------------------------------------------------"
echo " Excluding testing-utils feature of pallet currency;"
echo " that is for nightly version."
echo "------------------------------------------------------------------------"
cargo $sub_cmd -p currency --features std,testing-constants,try-runtime,runtime-benchmarks $options
fi
# run the clients
./scripts/cmd-clients "$reason" "$sub_cmd" "$options"

cargo $sub_cmd -p fee --all-features $options
cargo $sub_cmd -p issue --all-features $options
cargo $sub_cmd -p nomination --all-features $options
cargo $sub_cmd -p oracle --all-features $options
cargo $sub_cmd -p pooled-rewards --all-features $options
cargo $sub_cmd -p redeem --all-features $options
cargo $sub_cmd -p replace --all-features $options
cargo $sub_cmd -p reward-distribution --all-features $options
cargo $sub_cmd -p security --all-features $options
cargo $sub_cmd -p staking --all-features $options
cargo $sub_cmd -p stellar-relay --all-features $options
cargo $sub_cmd -p vault-registry --all-features $options

echo "------------------------------------------------------------------------"
echo "Checks" $reason "for pallets with rpc"
echo "------------------------------------------------------------------------"
cargo $sub_cmd -p module-issue-rpc --all-features $options
cargo $sub_cmd -p module-oracle-rpc --all-features $options
cargo $sub_cmd -p module-redeem-rpc --all-features $options
cargo $sub_cmd -p module-replace-rpc --all-features $options
cargo $sub_cmd -p module-vault-registry-rpc --all-features $options
cargo $sub_cmd -p module-issue-rpc-runtime-api --all-features $options
cargo $sub_cmd -p module-oracle-rpc-runtime-api --all-features $options
cargo $sub_cmd -p module-redeem-rpc-runtime-api --all-features $options
cargo $sub_cmd -p module-replace-rpc-runtime-api --all-features $options
cargo $sub_cmd -p module-vault-registry-rpc-runtime-api --all-features $options
# run the others
./scripts/cmd-others $reason $sub_cmd $options
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we sometimes wrap it with "" and sometimes we don't? Like, "$reason" vs $reason

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, there is no underlying reason. I removed the middle ""

Comment on lines 4 to 13
##### Executes the command to ALL packages of this project.
##### An exception is the package `subxt-client`, as it needs the package `runtime` for it to work.
##### The currently supported (and tested) commands are: check and clippy
##### arguments:
##### * reason = what the command is for
##### * sub_cmd = the actual sub command. For commands separated by spaces, enclose with ""
##### e.g. "build --lib"
##### "+nightly check"
##### * options = additional options of the sub command. If separated by spaces, enclose with ""
##### e.g. "-- -W clippy::all"
Copy link
Member

Choose a reason for hiding this comment

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

The comment here is out of place and should be only in cmd-all right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. removed

@b-yap b-yap merged commit 7c79898 into main Oct 3, 2024
2 of 4 checks passed
@b-yap b-yap deleted the fix-ci branch October 3, 2024 11:00
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.

2 participants