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

[Merged by Bors] - update dependencies #4639

Closed
wants to merge 11 commits into from
Closed

Conversation

jxs
Copy link
Member

@jxs jxs commented Aug 18, 2023

Issue Addressed

updates underlying dependencies and removes the ignored RUSTSEC's for cargo audit.

Also switches procinfo to procfs on eth2 to remove the nom warning, procinfo is unmaintained see here.

@jxs jxs force-pushed the update-deps branch 4 times, most recently from c60f021 to ea4b05b Compare August 18, 2023 16:08
to remove nom warning
@jxs jxs force-pushed the update-deps branch 5 times, most recently from 46c1594 to a410ede Compare August 21, 2023 23:06
on beacon_node and validator_client tests
- fix tls test by adding cert and key file
@jimmygchen jimmygchen added ready-for-review The code is ready for review dependencies Pull requests that update a dependency file labels Aug 22, 2023
@jimmygchen
Copy link
Member

Nice, looks like this PR addresses both #2727 and #4623 🎉

@@ -260,6 +260,7 @@ fn http_flag() {
fn http_address_flag() {
let addr = "127.0.0.99".parse::<IpAddr>().unwrap();
CommandLineTest::new()
.flag("http", None)
Copy link
Member

Choose a reason for hiding this comment

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

Are these added to fix some new failing tests?
It makes sense but I'm just asking because the cli definition doesn't actually require "http" to be present when the below flags are used (e.g. "http-address" arg doesn't have .require("http"))

.arg(
Arg::with_name("http-address")
.long("http-address")
.value_name("ADDRESS")
.help("Set the address for the HTTP address. The HTTP server is not encrypted \
and therefore it is unsafe to publish on a public network. When this \
flag is used, it additionally requires the explicit use of the \
`--unencrypted-http-transport` flag to ensure the user is aware of the \
risks involved. For access via the Internet, users should apply \
transport-layer security like a HTTPS reverse-proxy or SSH tunnelling.")
.requires("unencrypted-http-transport"),
)
.arg(
Arg::with_name("unencrypted-http-transport")
.long("unencrypted-http-transport")
.help("This is a safety flag to ensure that the user is aware that the http \
transport is unencrypted and using a custom HTTP address is unsafe.")
.requires("http-address"),
)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, thanks for bringing this up Jimmy. without the http and the metrics flag the both servers don't run,
see here for the validator client http server:

self.http_api_listen_addr = if self.config.http_api.enabled {
let ctx = Arc::new(http_api::Context {
task_executor: self.context.executor.clone(),
api_secret,
validator_store: Some(self.validator_store.clone()),
validator_dir: Some(self.config.validator_dir.clone()),
graffiti_file: self.config.graffiti_file.clone(),
graffiti_flag: self.config.graffiti,
spec: self.context.eth2_config.spec.clone(),
config: self.config.http_api.clone(),
sse_logging_components: self.context.sse_logging_components.clone(),
slot_clock: self.slot_clock.clone(),
log: log.clone(),
_phantom: PhantomData,
});

and here for the metrics:
// Optionally start the metrics server.
let http_metrics_ctx = if config.http_metrics.enabled {
let shared = http_metrics::Shared {
validator_store: None,
genesis_time: None,
duties_service: None,
};
let ctx: Arc<http_metrics::Context<T>> = Arc::new(http_metrics::Context {
config: config.http_metrics.clone(),
shared: RwLock::new(shared),
log: log.clone(),
});
let exit = context.executor.exit();
let (_listen_addr, server) = http_metrics::serve(ctx.clone(), exit)
.map_err(|e| format!("Unable to start metrics API server: {:?}", e))?;
context
.clone()
.executor
.spawn_without_exit(server, "metrics-api");
Some(ctx)
} else {
info!(log, "HTTP metrics server is disabled");
None
};

I think I commented on mattermost that we should probably make all these flags dependent on the main http and metrics ones, I didn't know it was that simple, I can do that in a subsequent PR if everyone agrees.

Copy link
Member

Choose a reason for hiding this comment

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

These tests actually just verify the parsed configs. The CommandLineTest starts the lighthouse process, dump the loaded config and shuts down immediately (it doesn't even create a ProductionBeaconNode or ProductionValidatorClient). But if were to make these flags required, we'll need them here. Updating in a subsequent PR is fine by me!

cmd.arg("--datadir")
.arg(tmp_dir.path().as_os_str())
.arg(format!("--{}", "dump-config"))
.arg(tmp_config_path.as_os_str())
.arg(format!("--{}", "dump-chain-config"))
.arg(tmp_chain_config_path.as_os_str())
.arg("--immediate-shutdown");

Copy link
Member Author

Choose a reason for hiding this comment

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

These tests actually just verify the parsed configs.

Yeah but it was one of the tests that already had the metrics flag before that failed and allowed us to acknowledge the CORS bug after the warp update, that's why I added the flag to all the other tests.

But if were to make these flags required, we'll need them here. Updating in a subsequent PR is fine by me!

Ok nice, I'll do it then, thanks Jimmy

Copy link
Member

@paulhauner paulhauner left a comment

Choose a reason for hiding this comment

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

Nice, thanks! LGTM!

@paulhauner paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 28, 2023
@paulhauner
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 28, 2023
## Issue Addressed

updates underlying dependencies and removes the ignored `RUSTSEC`'s for `cargo audit`.

Also switches `procinfo` to `procfs` on `eth2` to remove the `nom` warning, `procinfo` is unmaintained see [here](danburkert/procinfo-rs#46).
@bors
Copy link

bors bot commented Aug 28, 2023

@bors bors bot changed the title update dependencies [Merged by Bors] - update dependencies Aug 28, 2023
@bors bors bot closed this Aug 28, 2023
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

following discussion on #4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
bors bot pushed a commit that referenced this pull request Sep 22, 2023
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after #4639 get's merged.
closes #4651


Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
@macladson
Copy link
Member

Just noting that this effectively fixes and implements both changes from #4287 and #4275 🎉

Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

following discussion on sigp#4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

Synchronize dependencies and edition on the workspace `Cargo.toml`

## Proposed Changes

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

## Additional Info
this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after sigp#4639 get's merged.
closes sigp#4651


Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
updates underlying dependencies and removes the ignored `RUSTSEC`'s for `cargo audit`.

Also switches `procinfo` to `procfs` on `eth2` to remove the `nom` warning, `procinfo` is unmaintained see [here](danburkert/procinfo-rs#46).
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

following discussion on sigp#4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Synchronize dependencies and edition on the workspace `Cargo.toml`

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after sigp#4639 get's merged.
closes sigp#4651

Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
updates underlying dependencies and removes the ignored `RUSTSEC`'s for `cargo audit`.

Also switches `procinfo` to `procfs` on `eth2` to remove the `nom` warning, `procinfo` is unmaintained see [here](danburkert/procinfo-rs#46).
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

following discussion on sigp#4639 (comment) this PR makes the `http` and `metrics` sub-flags to require those main flags enabled
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Synchronize dependencies and edition on the workspace `Cargo.toml`

with rust-lang/cargo#8415 merged it's now possible to synchronize details on the workspace `Cargo.toml` like the metadata and dependencies.
By only having dependencies that are shared between multiple crates aligned on the workspace `Cargo.toml` it's easier to not miss duplicate versions of the same dependency and therefore ease on the compile times.

this PR also removes the no longer required direct dependency of the `serde_derive` crate.

should be reviewed after sigp#4639 get's merged.
closes sigp#4651

Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Co-authored-by: Michael Sproul <micsproul@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file ready-for-merge This PR is ready to merge. v4.4.1 ETA August 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants