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 support for building only on a single system #640

Open
wants to merge 1 commit into
base: released
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,19 @@ the Nixpkgs checkout (see also "[How does ofborg call
Builds will run on all allowed machines. For more information, see the "[Trusted
Users](#trusted-users)" section.

### build_system

```
@ofborg build_system SYSTEM list of attrs
```

Same as [build](#build), but restricts building to only one platform specification
instead of attempting to build on all allowed and supported systems.
This can be helpful to reduce needless builds and noise when debugging a platform-specific issue.
Copy link
Member

Choose a reason for hiding this comment

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

The need for this roots from a different problem: people using ofborg for debugging which is not great. For every push the eval task runs which is rather expensive.

Copy link
Author

Choose a reason for hiding this comment

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

I'm a little conflicted about this. Certainly ofborg should not be the primary development tool while working on a PR. On the other hand, if you're trying to debug a build failure on a foreign architecture ofborg can be the only way to properly test a PR.
According to the docs and the code, it should be possible to skip evaluation by assigning the work-in-progress label to a PR. However, for some reason this does not seem to stop ofborg from running the eval task (see ofborg/src/tasks/evaluate.rs), at least when I tested it. Perhaps it would make sense to skip the eval task for all draft PRs.

Personally, I don't have access to any macOS systems and other than ofborg I don't know about another way to test whether a package builds on Darwin.


Only the currently supported Nixpkgs systems can be passed to `build_system`:
`x86_64-linux`, `aarch64-linux`, `x86_64-darwin`, and `aarch64-darwin`.

## Multiple Commands

You can use multiple commands in a variety ways. Here are some valid
Expand Down
100 changes: 83 additions & 17 deletions ofborg/src/commentparser.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::systems::System;
use nom::types::CompleteStr;
use tracing::warn;

Expand All @@ -24,32 +25,79 @@ named!(
|s: CompleteStr| !s.0.eq_ignore_ascii_case("@grahamcofborg")
)
);

named!(
parse_line_impl(CompleteStr) -> Option<Vec<Instruction>>,
system(CompleteStr) -> System,
alt!(
do_parse!(
res: ws!(many1!(ws!(preceded!(
alt!(tag_no_case!("@grahamcofborg") | tag_no_case!("@ofborg")),
alt!(
ws!(do_parse!(
tag!("build") >>
value!(System::X8664Linux, tag!("x86_64-linux")) |
value!(System::Aarch64Linux, tag!("aarch64-linux")) |
value!(System::X8664Darwin, tag!("x86_64-darwin")) |
value!(System::Aarch64Darwin, tag!("aarch64-darwin"))
)
);

named!(
invocation_prefix(CompleteStr) -> CompleteStr,
alt!(tag_no_case!("@ofborg") | tag_no_case!("@grahamcofborg"))
);

enum Command {
Eval,
Build,
BuildSystem,
Test,
}

named!(
command_str(CompleteStr) -> Option<Command>,
alt!(
value!(Some(Command::Eval), tag!("eval")) |
value!(Some(Command::BuildSystem), tag!("build_system")) |
value!(Some(Command::Build), tag!("build")) |
value!(Some(Command::Test), tag!("test")) |

// TODO: Currently keeping previous behaviour of ignoring unknown commands. Maybe
// it would be better to return an error so that the caller would know one of the
// commands couldn't be handled?
value!(None, many_till!(take!(1), invocation_prefix))
)
);

named!(
command(CompleteStr) -> Option<Instruction>,
preceded!(
ws!(invocation_prefix),
switch!( ws!(command_str),
Some(Command::Build) =>
ws!(do_parse!(
pkgs: ws!(many1!(map!(normal_token, |s| s.0.to_owned()))) >>
(Some(Instruction::Build(Subset::Nixpkgs, pkgs)))
)) |
Some(Command::BuildSystem) =>
ws!(do_parse!(
system: ws!(system) >>
pkgs: ws!(many1!(map!(normal_token, |s| s.0.to_owned()))) >>
(Some(Instruction::BuildOnSystem(system, Subset::Nixpkgs, pkgs)))
)) |
Some(Command::Test) =>
ws!(do_parse!(
tag!("test") >>
tests: ws!(many1!(map!(normal_token, |s| format!("nixosTests.{}", s.0)))) >>
(Some(Instruction::Build(Subset::Nixpkgs, tests)))
)) |
value!(Some(Instruction::Eval), tag!("eval")) |
// TODO: Currently keeping previous behaviour of ignoring unknown commands. Maybe
// it would be better to return an error so that the caller would know one of the
// commands couldn't be handled?
value!(None, many_till!(take!(1), tag_no_case!("@grahamcofborg")))
)
)))) >> eof!()
>> (Some(res.into_iter().flatten().collect()))
) | value!(None)
Some(Command::Eval) => ws!(do_parse!( (Some(Instruction::Eval)) )) |
None => do_parse!( (None) )
)
)
);

named!(
parse_line_impl(CompleteStr) -> Option<Vec<Instruction>>,
opt!(
do_parse!(
res: ws!(many1!(ws!(command)))
>> eof!()
>> (res.into_iter().flatten().collect())
)
)
);

Expand All @@ -68,6 +116,7 @@ pub fn parse_line(text: &str) -> Option<Vec<Instruction>> {
pub enum Instruction {
Build(Subset, Vec<String>),
Eval,
BuildOnSystem(System, Subset, Vec<String>),
}

#[allow(clippy::upper_case_acronyms)]
Expand Down Expand Up @@ -108,6 +157,23 @@ mod tests {
assert_eq!(None, parse("@grahamcofborg build"));
}

#[test]
fn build_system_comment() {
assert_eq!(
Some(vec![Instruction::BuildOnSystem(
System::X8664Linux,
Subset::Nixpkgs,
vec![String::from("foo")]
),]),
parse("@ofborg build_system x86_64-linux foo")
);
}

#[test]
fn unknown_system_comment() {
assert_eq!(None, parse("@ofborg build_system x86_64-foolinux foo"));
}

#[test]
fn eval_comment() {
assert_eq!(Some(vec![Instruction::Eval]), parse("@grahamcofborg eval"));
Expand Down
2 changes: 1 addition & 1 deletion ofborg/src/systems.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq, Eq)]
pub enum System {
X8664Linux,
Aarch64Linux,
Expand Down
30 changes: 30 additions & 0 deletions ofborg/src/tasks/githubcommentfilter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,36 @@ impl worker::SimpleWorker for GitHubCommentWorker {
},
));
}
commentparser::Instruction::BuildOnSystem(system, subset, attrs) => {
if !build_destinations.contains(&system) {
continue;
};
if subset == commentparser::Subset::NixOS && !system.can_run_nixos_tests() {
continue;
};

let msg = buildjob::BuildJob::new(
repo_msg.clone(),
pr_msg.clone(),
subset,
attrs,
None,
None,
format!("{}", Uuid::new_v4()),
);

let (exchange, routingkey) = system.as_build_destination();
response.push(worker::publish_serde_action(exchange, routingkey, &msg));

response.push(worker::publish_serde_action(
Some("build-results".to_string()),
None,
&buildjob::QueuedBuildJobs {
job: msg,
architectures: vec![system.to_string()],
},
));
}
commentparser::Instruction::Eval => {
let msg = evaluationjob::EvaluationJob {
repo: repo_msg.clone(),
Expand Down