-
Notifications
You must be signed in to change notification settings - Fork 400
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
Refactor Dune_engine.Targets #9407
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! All reviewed, benchmarked and battle-tested internally.
faa0d18
to
46011a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced the use of symlinking to create a directory target with just normal copying as an experiment and it didn't seem to help. I've also experimented with sandboxing to make sure that we specified all dependencies correctly and again without success.
The fact that doing a clean build gives us the right answer in the install rules is rather concerning and my first assumption is that there's some bug in the engine.
Unfortunately, I didn't get anywhere investigating. I'll keep investigating, but I don't think this should block the PR.
I've left a commit (which I later reverted) switching the symlinking to copying in case anyone wants to experiment.
8982e81
to
e5c880e
Compare
We had similar but subtly different code for sandboxed and non-sandboxed directory targets. This patch unifies the two. We also no longer promote empty (sub)directories. Previously, this was only the case for sandboxed directory targets because we didn't move empty (sub)directories from the sandbox to the build dir. Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
There was some code duplication here which gets in the way of subsequent refactoring. Let's get rid of it. Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
This is a pure refactoring. The main goal is to be able to move Dune_engine.Targets into a separate package which can be used from dune_cache. However, I believe that it also makes the code simpler to follow overall. The main change is that we move everything to do with async file operations into Build_system which really is the only user of those, anyway. In addition, we also slightly generalise [Targets.Produced.collect_digests] and remove an unused pretty-printing function. Signed-off-by: Roman Leshchinskiy <rleshchinskiy@janestreet.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
This reverts commit 46011a8. Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
e5c880e
to
92e776f
Compare
…cases When the promoted directory has sub-directories with only other sub-directories inside, the map will miss that directory and it will not be properly re-created, failing at children creation time. Regression in b1c339b / ocaml#9407 Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
…cases When the promoted directory has sub-directories with only other sub-directories inside, the map will miss that directory and it will not be properly re-created, failing at children creation time. I am not sure I fully follow the logic here (cc: @rleshchinskiy), for example, I did have trouble making a testcase fail without using `npm` to generate a large directory. Regression in b1c339b / ocaml#9407 Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
…cases When the promoted directory has sub-directories with only other sub-directories inside, the map will miss that directory and it will not be properly re-created, failing at children creation time. I am not sure I fully follow the logic here (cc: @rleshchinskiy), for example, I did have trouble making a testcase fail without using `npm` to generate a large directory. Regression in b1c339b / ocaml#9407 Closes ocaml#10609 . Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
…cases When the promoted directory has sub-directories with only other sub-directories inside, the map will miss that directory and it will not be properly re-created, failing at children creation time. I am not sure I fully follow the logic here (cc: @rleshchinskiy). The current test case works for me (TM), if we revert the fix we get: ``` Error: Cannot promote files to "node_modules/node-cmake/node_modules/ansi-regex". Reason: opendir(node_modules/node-cmake/node_modules/ansi-regex): No such file or directory ``` Regression in b1c339b / ocaml#9407 Closes ocaml#10609 . Signed-off-by: Emilio Jesus Gallego Arias <e+git@x80.org>
This is a bunch of refactoring around
Targets
and especiallyTargets.Produced
. The main changes are:Targets.Produced.collect_digests
which allows us to removeTargets.Produced.Option
.Build_system
which really is the only user of those, anyway.Dune_engine
fromTargets
. A later PR will move those into a separate library which will allow us to use them in the cache as well.