Run doc examples in parallel#3031
Run doc examples in parallel#3031maciektr wants to merge 3 commits intomaciektr/runnable-doc-snippetsfrom
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
maciektr
force-pushed
the
maciektr/runnable-doc-snippets
branch
from
f72aae9 to
a7398d5
Compare
maciektr
force-pushed
the
maciektr/runnable-doc-examples-parallel
branch
2 times, most recently
from
d03914e to
ee71d9c
Compare
maciektr
requested review from
Arcticae and
integraledelebesgue
and removed request for
a team
Arcticae
left a comment
Arcticae
left a comment
There was a problem hiding this comment.
LGTM, with some style suggestions from me
| target_dir: TempDir, | ||
| /// Build profile passed to `scarb build` / `scarb execute` and used to locate the | ||
| /// incremental cache directory (`target/<profile>/`). | ||
| profile: String, |
There was a problem hiding this comment.
It copies incremental cache to speed subsequent builds up - we need profile for it's location.
| Ok(()) | ||
| } | ||
|
|
||
| fn copy_dir_contents(from: &Path, to: &Path) -> Result<()> { |
There was a problem hiding this comment.
isn't this a recursive dir copy? why do we reimplement such stuff?
There was a problem hiding this comment.
Yep, we can use fs_extra crate too
| let strategy = block.run_strategy(); | ||
| let total_in_item = *blocks_per_item.get(&block.id.item_full_path).unwrap_or(&1); | ||
| let display_name = block.id.display_name(total_in_item); | ||
| let execution_blocks: Vec<(usize, &CodeBlock)> = indexed_blocks |
There was a problem hiding this comment.
Why isn't this typed as IndexedBlock as well?
| }, | ||
| Err(e) => { | ||
| _ => match run_results.remove(&order) { | ||
| Some(Ok(res)) => match res.status { |
There was a problem hiding this comment.
Three nested matches is a bit much for me
| Ok(r) if r.outcome != ExecutionOutcome::CompileError | ||
| ); | ||
| run_results.insert(*order, result); | ||
| if build_succeeded { |
There was a problem hiding this comment.
I'd need to do a clone on result then 😓
| // Run non-compile_fail blocks sequentially until the first successful compilation to warm | ||
| // the incremental cache. Compile_fail blocks are skipped here and always run in parallel. | ||
| for (order, block) in &execution_blocks { | ||
| if block.expected_outcome() == ExecutionOutcome::CompileError { |
There was a problem hiding this comment.
[nit]: Don't we want to filter out those beforehand? I would do it in a functional manner here
There was a problem hiding this comment.
We don't!
This loop is a trick we do - we compile the first block that compiles without an error, only for it to produce the incremental caches we can copy to all subsequent builds.
So we cannot filter builds that are expected to fail out, as we still need to compile (and assert they actually fail) later only. We skip them here, as they won't produce incremental caches we won't to grab if they fail.
| .filter(|(order, _)| !run_results.contains_key(order)) | ||
| .collect_vec() | ||
| .into_par_iter() | ||
| .map(|(order, block)| { |
There was a problem hiding this comment.
This could be extracted into its' own function
| pub fn new(metadata: &'a AdditionalMetadata, has_lib_target: bool, ui: Ui) -> Result<Self> { | ||
| let target_dir = | ||
| tempdir().context("failed to create directory for doc tests target directory")?; | ||
| let profile = std::env::var("SCARB_PROFILE").unwrap_or_else(|_| "dev".to_string()); |
There was a problem hiding this comment.
Why bother having it as a parameter here if we're getting this from env anyway
There was a problem hiding this comment.
I think having a centralized source for this is fine 🤔 Don't see a problem here
maciektr
force-pushed
the
maciektr/runnable-doc-snippets
branch
from
a7398d5 to
4bc11cc
Compare
maciektr
force-pushed
the
maciektr/runnable-doc-examples-parallel
branch
from
ee71d9c to
df4546c
Compare
2 participants
No description provided.