Log instantiation failures #134
Conversation
|
Bug: the snippet log isn't being sent when no attempt is made. |
ofborg/src/tasks/build.rs
Outdated
| @@ -94,6 +96,10 @@ impl<'a, 'b> JobActions<'a, 'b> { | |||
| }; | |||
| } | |||
|
|
|||
| pub fn log_snippet(&self) -> VecDeque<String> { | |||
| self.snippet_log.clone() | |||
There was a problem hiding this comment.
What about returning a &VecDeque<String> (with &self.snippet_log), so that the caller can clone() if need be?
There was a problem hiding this comment.
I had actually already changed this to return a Vec to be easier to pass to the build-result message.
| @@ -55,6 +55,7 @@ pub struct JobActions<'a, 'b> { | |||
| receiver: &'a mut notifyworker::NotificationReceiver, | |||
| job: &'b buildjob::BuildJob, | |||
| line_counter: u64, | |||
| snippet_log: VecDeque<String>, | |||
There was a problem hiding this comment.
A ring buffer would likely make more sense, both from a performance and a code simplicity standpoint, but maybe the added dependency isn't worth it?
There was a problem hiding this comment.
Yeah, a ring buffer would be nice here... perhaps an easy thing for a newbie to contribute (cc @LnL7)
There was a problem hiding this comment.
I'm spending most of my time on ZHF right now, but that sounds like a fun distraction 😄
There was a problem hiding this comment.
https://doc.rust-lang.org/std/collections/vec_deque/
This queue has O(1) amortized inserts and removals from both ends of the container.
There was a problem hiding this comment.
Yeah, the main reason for wanting a ring buffer is it means it'd allow to avoid having to manually maintain the invariant by hand, not performance (which, if I remember correctly how deques are implemented, should still be better in the constant, thanks to not allocating/deallocating memory frequently)
There was a problem hiding this comment.
(eg. https://github.com/NixOS/ofborg/pull/134/files/cfa9547434ea9b7f8666f6f140fc342f73741da9#diff-d9ced247c4766cbb09e7fe716d230b20R172 for the manually-maintaining-the-invariant point)
ofborg/src/tasks/build.rs
Outdated
| println!("----->8-----"); | ||
|
|
||
| let last10lines: Vec<String> = snippet_log.into_iter().collect::<Vec<String>>(); | ||
| let last10lines: Vec<String> = actions.log_snippet().into_iter().collect::<Vec<String>>(); |
There was a problem hiding this comment.
If following my above-mentioned advice of returning an &VecDeque<_> from log_snippet(), this should just become .iter(), so that the returned value isn't consumed. Also, the ::<Vec<String>> isn't required if there's the : Vec<String> type annotation on the variable declaration, only one is needed for type inference to work, usually (and here I'd guess none is actually needed, as the last10lines variable is then passed to a function, thus forcing its type).
ofborg/src/nix.rs
Outdated
| Vec<Result<String, (String, Vec<String>)>>, | ||
| Vec<Result<String, (String, Vec<String>)>>) = attr_instantiations | ||
| .into_iter() | ||
| .partition(|x| x.is_ok()); |
There was a problem hiding this comment.
I think this could be shortened (simplified?) into let (ok, err): (Vec<_>, Vec<_>) = attr_instantiations.into_iter().partition(|x| x.is_ok());
EDIT: actually, see below, this may not be actually a useful point to make if this part is to be deleted anyway.
ofborg/src/nix.rs
Outdated
| return ( | ||
| ok_ret, | ||
| err_ret | ||
| ) |
There was a problem hiding this comment.
Here I think it'd be cleaner (and likely much faster, though that's not the point) to just not try using iterators, and have let mut ok_ret; let mut err_ret;, do a big for look on the attrs, and just push to both vectors.
Something like (untested & typed in the github comment box):
let mut ok = Vec::new();
let mut err = Vec::new();
for attr in attr_instantiations.into_iter() {
match self.safely_instantiate_attrs(nixpkgs, file, vec![attr.clone()]) {
Ok(_) => { ok.push(attr); }
Err(f) => { err.push((attr, lines_from_file(f))); }
}
}
(ok, err)(also, it's standard practice to omit the return keyword where possible)
No description provided.