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

Add --partition M/N to distribute builds #253

Merged
merged 6 commits into from
Aug 8, 2024
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
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,9 @@ OPTIONS:
--keep-going
Keep going on failure.

--partition <M/N>
Partition runs and execute only its subset according to M/N.

--log-group <KIND>
Log grouping: none, github-actions.

Expand Down
10 changes: 9 additions & 1 deletion src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use lexopt::{
ValueExt,
};

use crate::{term, version::VersionRange, Feature, LogGroup, Rustup};
use crate::{term, version::VersionRange, Feature, LogGroup, Partition, Rustup};

pub(crate) struct Args {
pub(crate) leading_args: Vec<String>,
Expand Down Expand Up @@ -53,6 +53,8 @@ pub(crate) struct Args {
pub(crate) clean_per_version: bool,
/// --keep-going
pub(crate) keep_going: bool,
/// --partition
pub(crate) partition: Option<Partition>,
/// --print-command-list
pub(crate) print_command_list: bool,
/// --version-range/--rust-version
Expand Down Expand Up @@ -155,6 +157,7 @@ impl Args {
let mut clean_per_run = false;
let mut clean_per_version = false;
let mut keep_going = false;
let mut partition = None;
let mut print_command_list = false;
let mut no_manifest_path = false;
let mut locked = false;
Expand Down Expand Up @@ -308,6 +311,7 @@ impl Args {
Long("clean-per-run") => parse_flag!(clean_per_run),
Long("clean-per-version") => parse_flag!(clean_per_version),
Long("keep-going") => parse_flag!(keep_going),
Long("partition") => parse_opt!(partition, false),
Long("print-command-list") => parse_flag!(print_command_list),
Long("no-manifest-path") => parse_flag!(no_manifest_path),
Long("locked") => parse_flag!(locked),
Expand Down Expand Up @@ -571,6 +575,8 @@ impl Args {
None => LogGroup::auto(),
};

let partition = partition.as_deref().map(str::parse).transpose()?;

if no_dev_deps || no_private {
let flag = if no_dev_deps && no_private {
"--no-dev-deps and --no-private modify"
Expand Down Expand Up @@ -620,6 +626,7 @@ impl Args {
clean_per_run,
clean_per_version,
keep_going,
partition,
print_command_list,
no_manifest_path,
include_features: include_features.into_iter().map(Into::into).collect(),
Expand Down Expand Up @@ -813,6 +820,7 @@ const HELP: &[HelpText<'_>] = &[
"This flag can only be used together with --version-range flag.",
]),
("", "--keep-going", "", "Keep going on failure", &[]),
("", "--partition", "<M/N>", "Partition runs and execute only its subset according to M/N", &[]),
("", "--log-group", "<KIND>", "Log grouping: none, github-actions", &[
"If this option is not used, the environment will be automatically detected."
]),
Expand Down
39 changes: 37 additions & 2 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,22 @@ impl FromStr for LogGroup {
}
}

pub(crate) struct Partition {
index: usize,
count: usize,
}

impl FromStr for Partition {
type Err = Error;

fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
match s.split('/').map(str::parse::<usize>).collect::<Vec<_>>()[..] {
[Ok(index), Ok(count)] if 0 < index && index <= count => Ok(Self { index, count }),
_ => bail!("bad or out-of-range partition: {s}"),
}
}
}

fn exec_cargo(
cx: &Context,
id: &PackageId,
Expand Down Expand Up @@ -672,7 +688,26 @@ fn exec_cargo_inner(
if progress.count != 0 && !cx.print_command_list && cx.log_group == LogGroup::None {
eprintln!();
}
progress.count += 1;

let new_count = progress.count + 1;
let mut skip = false;
if let Some(partition) = &cx.partition {
if progress.count % partition.count != partition.index - 1 {
Copy link
Contributor Author

@ryoqun ryoqun Jul 16, 2024

Choose a reason for hiding this comment

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

this again subjective as well. but it's more intuitive not to see adjusted progress (i.e. {X/N}/{Y/N}) after partitioning while showing the skip messages.

let mut msg = String::new();
Copy link
Owner

Choose a reason for hiding this comment

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

IIUC this would be something like:

1/4 run
2/4 skipped
3/4 run
4/4 skipped

I think this is fine for the nextest use, I guess it maybe a bit inefficient for cargo-hack use, since the feature combinations that would reduce the amount of recompilation are often before or after the current combination.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for nice suggestion. I've switched from interleaved partitioning to chunked partitioning. and seems our ci got quicker to finish: https://buildkite.com/anza/agave/builds/7952#0190d3f9-52af-4b75-9e69-e408f4a1a57e

if term::verbose() {
write!(msg, "skipping {line}").unwrap();
} else {
write!(msg, "skipping {line} on {}", cx.packages(id).name).unwrap();
}
write!(msg, " ({}/{})", new_count, progress.total).unwrap();
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 may be subjective, but i liked explicitly printing messages about being skipped.

let _guard = cx.log_group.print(&msg);
skip = true;
}
}
progress.count = new_count;
if skip {
return Ok(());
}

if cx.clean_per_run {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(unrelated to this pr: i don't think cargo_clean(...) is needed if cx.print_command_list == true..right?)

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah. Well, both are optional and I don't know what the use case of using --clean-per-run together with -pr-int-command-list in the first place, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for quick reply. I'll create a small pr after this pr to fix this.

cargo_clean(cx, Some(id))?;
Expand All @@ -690,7 +725,7 @@ fn exec_cargo_inner(
} else {
write!(msg, "running {line} on {}", cx.packages(id).name).unwrap();
}
write!(msg, " ({}/{})", progress.count, progress.total).unwrap();
write!(msg, " ({}/{})", new_count, progress.total).unwrap();
let _guard = cx.log_group.print(&msg);

line.run()
Expand Down
3 changes: 3 additions & 0 deletions tests/long-help.txt
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ OPTIONS:
--keep-going
Keep going on failure.

--partition <M/N>
Partition runs and execute only its subset according to M/N.

--log-group <KIND>
Log grouping: none, github-actions.

Expand Down
2 changes: 2 additions & 0 deletions tests/short-help.txt
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ OPTIONS:
command
--clean-per-version Remove artifacts per Rust version
--keep-going Keep going on failure
--partition <M/N> Partition runs and execute only its subset according to
M/N
--log-group <KIND> Log grouping: none, github-actions
--print-command-list Print commands without run (Unstable)
--no-manifest-path Do not pass --manifest-path option to cargo (Unstable)
Expand Down