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.

Sign up for GitHub

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

Jump to bottom

Evaluate build paths in the context of the build's bindings #94

Merged
merged 3 commits into from
Jan 18, 2024
Merged

Evaluate build paths in the context of the build's bindings #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abcd3bb
Enforce only valid rule variables are used
Colecf Jan 3, 2024
5775ec2
Add benchmark test for the loader
Colecf Dec 31, 2023
d510d1d
Evaluate build paths in the context of the build's bindings
Colecf Dec 28, 2023
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
33 changes: 19 additions & 14 deletions benches/parse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use criterion::{criterion_group, criterion_main, Criterion};
use std::io::Write;
use n2::parse::Loader;
use std::{io::Write, path::PathBuf, str::FromStr};

pub fn bench_canon(c: &mut Criterion) {
// TODO switch to canon_path_fast
Expand All @@ -21,16 +22,9 @@ pub fn bench_canon(c: &mut Criterion) {
});
}

struct NoOpLoader {}
impl n2::parse::Loader for NoOpLoader {
type Path = ();
fn path(&mut self, _path: &mut str) -> Self::Path {
()
}
}

fn generate_build_ninja() -> Vec<u8> {
let mut buf: Vec<u8> = Vec::new();
write!(buf, "rule cc\n command = touch $out",).unwrap();
for i in 0..1000 {
write!(
buf,
Expand All @@ -44,14 +38,13 @@ fn generate_build_ninja() -> Vec<u8> {
}

fn bench_parse_synthetic(c: &mut Criterion) {
let mut loader = NoOpLoader {};
let mut input = generate_build_ninja();
input.push(0);
c.bench_function("parse synthetic build.ninja", |b| {
b.iter(|| {
let mut parser = n2::parse::Parser::new(&input);
loop {
if parser.read(&mut loader).unwrap().is_none() {
if parser.read().unwrap().is_none() {
break;
}
}
Expand All @@ -60,13 +53,12 @@ fn bench_parse_synthetic(c: &mut Criterion) {
}

fn bench_parse_file(c: &mut Criterion, mut input: Vec<u8>) {
let mut loader = NoOpLoader {};
input.push(0);
c.bench_function("parse benches/build.ninja", |b| {
b.iter(|| {
let mut parser = n2::parse::Parser::new(&input);
loop {
if parser.read(&mut loader).unwrap().is_none() {
if parser.read().unwrap().is_none() {
break;
}
}
Expand All @@ -85,5 +77,18 @@ pub fn bench_parse(c: &mut Criterion) {
};
}

criterion_group!(benches, bench_canon, bench_parse);
fn bench_load_synthetic(c: &mut Criterion) {
let mut input = generate_build_ninja();
input.push(0);
c.bench_function("load synthetic build.ninja", |b| {
b.iter(|| {
let mut loader = n2::load::Loader::new();
loader
.parse(PathBuf::from_str("build.ninja").unwrap(), &input)
.unwrap();
})
});
}

criterion_group!(benches, bench_canon, bench_parse, bench_load_synthetic);
criterion_main!(benches);
98 changes: 67 additions & 31 deletions src/eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@
//! `c++ $in -o $out`, and mechanisms for expanding those into plain strings.

use crate::smallmap::SmallMap;
use std::borrow::Borrow;
use std::{borrow::Cow, collections::HashMap};

/// An environment providing a mapping of variable name to variable value.
/// A given EvalString may be expanded with multiple environments as possible
/// context.
/// This represents one "frame" of evaluation context, a given EvalString may
/// need multiple environments in order to be fully expanded.
pub trait Env {
fn get_var(&self, var: &str) -> Option<Cow<str>>;
fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>>;
}

/// One token within an EvalString, either literal text or a variable reference.
#[derive(Debug)]
#[derive(Debug, Clone, PartialEq)]
pub enum EvalPart<T: AsRef<str>> {
Literal(T),
VarRef(T),
Expand All @@ -22,29 +23,38 @@ pub enum EvalPart<T: AsRef<str>> {
/// This is generic to support EvalString<&str>, which is used for immediately-
/// expanded evals, like top-level bindings, and EvalString<String>, which is
/// used for delayed evals like in `rule` blocks.
#[derive(Debug)]
#[derive(Debug, PartialEq)]
pub struct EvalString<T: AsRef<str>>(Vec<EvalPart<T>>);
impl<T: AsRef<str>> EvalString<T> {
pub fn new(parts: Vec<EvalPart<T>>) -> Self {
EvalString(parts)
}

pub fn evaluate(&self, envs: &[&dyn Env]) -> String {
let mut val = String::new();
fn evaluate_inner(&self, result: &mut String, envs: &[&dyn Env]) {
for part in &self.0 {
match part {
EvalPart::Literal(s) => val.push_str(s.as_ref()),
EvalPart::Literal(s) => result.push_str(s.as_ref()),
EvalPart::VarRef(v) => {
for env in envs {
for (i, env) in envs.iter().enumerate() {
if let Some(v) = env.get_var(v.as_ref()) {
val.push_str(&v);
v.evaluate_inner(result, &envs[i + 1..]);
break;
}
}
}
}
}
val
}

/// evalulate turns the EvalString into a regular String, looking up the
/// values of variable references in the provided Envs. It will look up
/// its variables in the earliest Env that has them, and then those lookups
/// will be recursively expanded starting from the env after the one that
Copy link
Owner

Choose a reason for hiding this comment

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

I believe this isn't right.

The Ninja docs aren't too great but the intent is expansion only ever happens once:
https://ninja-build.org/manual.html#_variable_expansion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is mostly an implementation detail. I mostly made this change so that it was easy to turn an EvalString into the proper string under a variety of different circumstances. (rule bindings vs build bindings) If we want, we can expand out variables at any point in the process.

However I don't think doing early expansion will really help that much. Consider this build.ninja:

root_out = out

build ${local_out}/foo: ${local_out}/bar
    local_out = ${root_out}/local

Here, we have to expand 2 paths. Even if we had preemptively flattened root_out into local_out, we still need to copy the out/local path fragment into two separate strings. So flattening it doesn't save us any string copying time, only some scope traversal time, but since there are a maximum of 4 scopes currently in n2 (implicit, rule, build, global, though this will increase when subninja support is added), I don't think that matters.

Copy link
Owner

Choose a reason for hiding this comment

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

That is an interesting argument regarding scope traversal time!

I think the most nested case of lookup (excluding subninja) is a file

var = a

rule print
  command = echo "var:'$var' out:'$out'"

build c$var: print
  var = b$var

which prints
var:'ba' out:'cba'

/// had the first successful lookup.
pub fn evaluate(&self, envs: &[&dyn Env]) -> String {
let mut result = String::new();
self.evaluate_inner(&mut result, envs);
result
}
}

Expand All @@ -62,6 +72,34 @@ impl EvalString<&str> {
}
}

impl EvalString<String> {
pub fn as_cow(&self) -> EvalString<Cow<str>> {
EvalString(
self.0
.iter()
.map(|part| match part {
EvalPart::Literal(s) => EvalPart::Literal(Cow::Borrowed(s.as_ref())),
EvalPart::VarRef(s) => EvalPart::VarRef(Cow::Borrowed(s.as_ref())),
})
.collect(),
)
}
}

impl EvalString<&str> {
pub fn as_cow(&self) -> EvalString<Cow<str>> {
EvalString(
self.0
.iter()
.map(|part| match part {
EvalPart::Literal(s) => EvalPart::Literal(Cow::Borrowed(*s)),
EvalPart::VarRef(s) => EvalPart::VarRef(Cow::Borrowed(*s)),
})
.collect(),
)
}
}

/// A single scope's worth of variable definitions.
#[derive(Debug, Default)]
pub struct Vars<'text>(HashMap<&'text str, String>);
Expand All @@ -70,36 +108,34 @@ impl<'text> Vars<'text> {
pub fn insert(&mut self, key: &'text str, val: String) {
self.0.insert(key, val);
}
pub fn get(&self, key: &'text str) -> Option<&String> {
pub fn get(&self, key: &str) -> Option<&String> {
self.0.get(key)
}
}
impl<'a> Env for Vars<'a> {
fn get_var(&self, var: &str) -> Option<Cow<str>> {
self.0.get(var).map(|str| Cow::Borrowed(str.as_str()))
fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
Some(EvalString::new(vec![EvalPart::Literal(
std::borrow::Cow::Borrowed(self.get(var)?),
)]))
}
}

impl<K: Borrow<str> + PartialEq> Env for SmallMap<K, EvalString<String>> {
fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
Some(self.get(var)?.as_cow())
}
}

// Impl for Loader.rules
impl Env for SmallMap<String, EvalString<String>> {
fn get_var(&self, var: &str) -> Option<Cow<str>> {
// TODO(#83): this is wrong in that it doesn't include envs.
// This can occur when you have e.g.
// rule foo
// bar = $baz
Copy link
Owner

Choose a reason for hiding this comment

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

In Ninja it turns out you can't declare arbitrary vars in a rule block so this example isn't legal.

I tried a construction like

rule print
  command = echo "varref is $varref"
  depfile = abc
build out: print
  varref = $depfile

And Ninja expanded varref to the empty string.

Copy link
Owner

Choose a reason for hiding this comment

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

...which I think means this worry was misplaced (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand this comment, (where is x used?) but just from the signature you can tell it was wrong, (You can't convert an EvalString to a String without any additional input, or else you shouldn't have used an EvalString in the first place) so I reworked it so Envs return EvalStrings.

// build ...: foo
// x = $bar
// When evaluating the value of `x`, we find `bar` in the rule but
// then need to pick the right env to evaluate $baz. But we also don't
// wanna generically always use all available envs because we don't
// wanna get into evaluation cycles.
self.get(var).map(|val| Cow::Owned(val.evaluate(&[])))
impl<K: Borrow<str> + PartialEq> Env for SmallMap<K, EvalString<&str>> {
fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
Some(self.get(var)?.as_cow())
}
}

// Impl for the variables attached to a build.
impl Env for SmallMap<&str, String> {
fn get_var(&self, var: &str) -> Option<Cow<str>> {
self.get(var).map(|val| Cow::Borrowed(val.as_str()))
fn get_var(&self, var: &str) -> Option<EvalString<Cow<str>>> {
Some(EvalString::new(vec![EvalPart::Literal(
std::borrow::Cow::Borrowed(self.get(var)?),
)]))
}
}
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ mod depfile;
mod eval;
mod graph;
mod hash;
mod load;
pub mod load;
pub mod parse;
mod process;
#[cfg(unix)]
Expand Down
Loading
Loading