Introduce scheme #2086
Introduce scheme #2086
Conversation
aalekseyev
force-pushed
the
introduce-scheme
branch
4 times, most recently
from
de44ec5 to
57ad0ee
Compare
aalekseyev
force-pushed
the
introduce-scheme
branch
3 times, most recently
from
dcd255e to
dccb094
Compare
aalekseyev
force-pushed
the
introduce-scheme
branch
from
dccb094 to
4a884c6
Compare
|
BTW, would it be hard to extract 0a9c83a into a separate PR? It looks independant from this PR. |
|
@diml, I don't think it's independent. I could move it together with the rest of install_rules changes easily enough though. That would leave scheme unused in this feature. |
|
I will give this a review today. Sorry for the delay. Splitting this PR would be highly appreciated and make review easier for me. |
aalekseyev
force-pushed
the
introduce-scheme
branch
from
e11a3d3 to
c0b8b8b
Compare
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
- add more doc and try to make the API more "progressive", i.e. going from simple to complex concepts - reduce the number of intermediate maps in the various operations - use a more compact representation - rename `intersect` to `inter` to match `Set`'s API Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
…uite Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
aalekseyev
force-pushed
the
introduce-scheme
branch
2 times, most recently
from
fbecd1e to
0662dc6
Compare
|
Moved the install rules changes to #2130. I also squashed much of the early history of the feature. |
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
| in | ||
| fun x ~of_ -> | ||
| match loop x of_ with | ||
| | (_ : t) -> true |
There was a problem hiding this comment.
Why do we return t here and not unit for example?
There was a problem hiding this comment.
It was just so that I could write merge... ~f:loop which wants f to return a t.
| | Approximation (paths, rules) -> | ||
| if | ||
| not (Dir_set.is_subset paths ~of_:env) | ||
| && not (Dir_set.is_subset (Dir_set.negate paths) ~of_:env) |
There was a problem hiding this comment.
What is this env parameter for? It seems to always be Universal, so these is_subset calls will always be true. Do we expect it to vary down the line?
There was a problem hiding this comment.
Ah, that's a bug. Thanks for catching. I pushed a fix.
The purpose of the parameter is to catch errors when rules are generated for directories outside of env. Speaking of which, that's not checked in the Finite branch at all.
I'm making a fix for that too.
| | Union of 'rules t * 'rules t | ||
| | Approximation of Dir_set.t * 'rules t | ||
| | Finite of 'rules Path.Build.Map.t | ||
| | Thunk of (unit -> 'rules t) |
There was a problem hiding this comment.
I would add a comment to each of these constructors.
Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
|
@rgrinberg, the utility is having fewer PRs in flight at a time :-) and having github understand what the diff of the follow-up PRs is. |
* Introduce Scheme Signed-off-by: Jeremie Dimino <jeremie@dimino.org> Signed-off-by: Arseniy Alekseyev <aalekseyev@janestreet.com>
Scheme.tfor expressing lazily generated subtree rules.Rules.trepresentation is ugly and temporary. See Move rules collector out of build system #2089 where we turn it into a passive data structure instead.