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

fix: Correct dependencies and build checks #484

Merged
merged 9 commits into from
Nov 14, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 6 additions & 6 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ jobs:
uses: actions-rs/cargo@v1
with:
command: check
args: --all --bins --tests --examples
args: --workspace --all-features --bins --tests --examples --benches
Copy link
Contributor

Choose a reason for hiding this comment

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

why having --all-features is good to test against, we also need to have tests (or at least build checks) for each individual feature, to make sure they work by themselves and with no features enabled at all

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, that's true. I was trying to take small steps.

Given how difficult it was to get this to be happy in CI I'd prefer to do that as a followup since it already fixes some things on main and would stop regressions to that instead of having to keep playing catchup with main while working on this - as already happened.

Copy link
Contributor

Choose a reason for hiding this comment

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

this was not meant to add more work to this pr, just a reminder for all of us that it needs more work 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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


- name: tests
uses: actions-rs/cargo@v1
Expand All @@ -145,17 +145,17 @@ jobs:
# GHA if this is not present
# https://twitter.com/steipete/status/921066430220652544?lang=en
# https://blog.phusion.nl/2017/10/13/why-ruby-app-servers-break-on-macos-high-sierra-and-what-can-be-done-about-it/
OBJC_DISABLE_INITIALIZE_FORK_SAFETY: YES
OBJC_DISABLE_INITIALIZE_FORK_SAFETY: "YES"
with:
command: test
args: --all -j 4
args: -j 4 --workspace --all-features --examples --benches --bins
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is -j 4 really needed though? I think cargo automatically figures out the number of cpu's and uses that. Or is the intention to use more parallelism then there are CPUs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh, welcome to the world of CI runners :) If you want to help things along it might make sense to drop down to -j 2 for a moment.

The reasoning is that most of these (esp. GHA runners) OOM very easily if you hit the tests/builds hard. Hence manually limiting to 4 and now potentially even lower for windows builds.

This should all go away once we move to our infra.


- name: clipy
- name: clippy
uses: actions-rs/cargo@v1
if: matrix.os == 'ubuntu-latest' && matrix.rust=='stable'
with:
command: clippy
args: --all --tests --benches --all-targets -- -D warnings
command: clippy
args: --workspace --all-features --tests --benches --examples --bins --all-targets -- -D warnings

- name: build release
uses: actions-rs/cargo@v1
Expand Down
6 changes: 3 additions & 3 deletions DEVELOPERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,15 @@ When you push your branch to github and open up a pull request, it will automati
In order to catch linting and testing errors before pushing the code to github, be sure to run:

```shell
$ cargo clippy --workspace --examples --benches --tests
$ cargo test --workspace --examples --benches
$ cargo clippy --workspace --all-features --examples --benches --tests --bins
$ cargo test --workspace --all-features --examples --benches --bins
```

Setting up a [git hook][git-hook] to run these commands can save you many headaches:

```shell
#!/bin/sh
cargo clippy --workspace --examples --tests --benches && cargo test --workspace --examples
cargo clippy --workspace --all-features --examples --benches --tests --bins && cargo test --workspace --all-features --examples --benches --bins
```

## <a name="dependecies"></a> Dependencies
Expand Down
2 changes: 1 addition & 1 deletion iroh-metrics/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ mod tests {
#[cfg(feature = "tokio-console")]
expect.insert(
"tokio_console".to_string(),
Value::new(None, cfg.tokio_console.clone()),
Value::new(None, cfg.tokio_console),
);
let got = cfg.collect().unwrap();
for key in got.keys() {
Expand Down
3 changes: 2 additions & 1 deletion iroh-one/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ iroh-store = {path = "../iroh-store", default-features = false, features = ["rpc
iroh-util = {path = "../iroh-util"}
reqwest = {version = "0.11", features = ["rustls-tls"], default-features = false}
serde = {version = "1.0", features = ["derive"]}
tempfile = { version = "3.3.0", optional = true }
tokio = {version = "1", features = ["macros", "rt-multi-thread", "process"]}
tracing = "0.1.33"

Expand All @@ -41,4 +42,4 @@ rpc-grpc = ["iroh-rpc-types/grpc", "iroh-rpc-client/grpc", "iroh-metrics/rpc-grp
rpc-mem = ["iroh-rpc-types/mem", "iroh-rpc-client/mem"]
# uds-gateway controls whether http over uds is enabled. This is independent of the
# rpc-grpc control endpoint.
uds-gateway = []
uds-gateway = ["tempfile"]
11 changes: 9 additions & 2 deletions iroh-one/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,9 @@ impl Config {
/// as a single entry point for other system services if feature enabled.
pub fn default_rpc_config() -> RpcClientConfig {
#[cfg(feature = "uds-gateway")]
let path: PathBuf = tempdir::TempDir::new("iroh")
let path: PathBuf = tempfile::Builder::new()
.prefix("iroh")
.tempfile()
.unwrap()
.path()
.join("ipfsd.http");
Expand Down Expand Up @@ -93,7 +95,12 @@ impl Config {
impl Default for Config {
fn default() -> Self {
#[cfg(feature = "uds-gateway")]
let gateway_uds_path: PathBuf = TempDir::new("iroh").unwrap().path().join("ipfsd.http");
let gateway_uds_path: PathBuf = tempfile::Builder::new()
.prefix("iroh")
.tempfile()
.unwrap()
.path()
.join("ipfsd.http");
let rpc_client = Self::default_rpc_config();
let metrics_config = MetricsConfig::default();
let store_config =
Expand Down
7 changes: 5 additions & 2 deletions iroh-one/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use iroh_rpc_types::Addr;
use iroh_util::lock::ProgramLock;
use iroh_util::{iroh_config_path, make_config};
#[cfg(feature = "uds-gateway")]
use tempdir::TempDir;
use tokio::sync::RwLock;

#[tokio::main(flavor = "multi_thread")]
Expand Down Expand Up @@ -100,7 +99,11 @@ async fn main() -> Result<()> {

#[cfg(feature = "uds-gateway")]
let uds_server_task = {
let mut path = TempDir::new("iroh")?.path().join("ipfsd.http");
let mut path = tempfile::Builder::new()
.prefix("iroh")
.tempdir()?
.path()
.join("ipfsd.http");
if let Some(uds_path) = config.gateway_uds_path {
path = uds_path;
} else {
Expand Down
2 changes: 1 addition & 1 deletion iroh-share/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ libp2p = { version = "0.50", default-features = false, features = ["gossipsub"]
multibase = "0.9.1"
rand = "0.8.5"
serde = { version = "1", features = ["derive"] }
tempfile = "3.3"
tempfile = "3.3.0"
tokio = { version = "1" }
tokio-stream = "0.1.9"
tracing = "0.1.34"
Expand Down