Conversation
bin/main.ml
Outdated
| let doc = "Clean the project." in | ||
| let man = | ||
| [ `S "DESCRIPTION" | ||
| ; `P {|Removes files added by jbuilder including _build and .merlin|} |
There was a problem hiding this comment.
Certainly! Great idea
bin/main.ml
Outdated
| in | ||
| let go common = | ||
| set_common common ~targets:[]; | ||
| let log = Log.create () in |
There was a problem hiding this comment.
We should probably not create the log, since it's stored in _build/log.
There was a problem hiding this comment.
Good point, I'll take it out.
bin/main.ml
Outdated
| Future.Scheduler.go ~log | ||
| begin | ||
| (* Attach this first before we run anything else *) | ||
| at_exit (fun () -> |
There was a problem hiding this comment.
Because there's another at_exit handler which dumps a a trace file called .db
Since the at_exit handlers are processed LIFO, I need this one to get in the stack first.
There was a problem hiding this comment.
Ok. In the future there will be a problem with the method used anyway, as computing rules might require running commands. This will be required to support plugins properly.
There is a simpler approach we could use here:
- delete the
_builddirectory - load the trace and delete the files that are in the source tree. The keys in Build_system.Trace.t are the targets generated since the initial build from a clean tree
Then we don't need to create the build system at all. You can expose a function in Build_system to read the trace and returns all the targets.
|
Thanks, can you also add |
|
So the only question I have now is, what do we do in the case when the trace file is missing/corrupt? Currently I'm erring on the safe side and just outputting an error message to let the user fix it, but I'm not particularly proud of that solution. Any ideas? |
bin/main.ml
Outdated
| let doc = "Clean the project." in | ||
| let man = | ||
| [ `S "DESCRIPTION" | ||
| ; `P {|Removes files added by jbuilder such as _build, .install, and .merlin|} |
There was a problem hiding this comment.
I'd prefer: "_build, <package>.install and .merlin."
bin/main.ml
Outdated
| | None -> | ||
| Format.eprintf | ||
| "@{<error>Error@}: Missing trace file: _build/.db\n\ | ||
| Hint: Is the _build directory corrupt? Try rebuilding? @." |
There was a problem hiding this comment.
No trace is likely to mean nothing was built yet. We should just ignore it.
bin/main.ml
Outdated
| | Some trace -> | ||
| begin | ||
| Hashtbl.iter trace ~f:(fun ~key ~data:(_) -> Path.unlink key); | ||
| Path.(rm_rf (append root (of_string "_build"))) |
There was a problem hiding this comment.
The _build directory could exist even though there is no trace. The removal of _build should be done unconditionally.
src/build_system.ml
Outdated
| trace | ||
| end | ||
|
|
||
| let load_trace () = Option.some_if (Sys.file_exists Trace.file) (Trace.load ()) |
There was a problem hiding this comment.
This doesn't really work. Both arguments will be evaluated before Option.some_if is called.
src/build_system.mli
Outdated
|
|
||
| val load_trace | ||
| : unit | ||
| -> (Path.t, string) Hashtbl.t option |
There was a problem hiding this comment.
The hash table doesn't make much sense outside of this module. We should just return the list of keys:
val all_targets_ever_built : unit -> string list
There was a problem hiding this comment.
Should that be string list or Path.t list?
bin/main.ml
Outdated
| Hint: Is the _build directory corrupt? Try rebuilding? @." | ||
| | Some trace -> | ||
| begin | ||
| Hashtbl.iter trace ~f:(fun ~key ~data:(_) -> Path.unlink key); |
There was a problem hiding this comment.
Some of the files might not exist anymore, for instance if the user renamed a directory there will still be an entry for the old .merlin file. We need to use Path.unlink_no_error.
|
If the trace is missing we can just ignore it. We can't know whether the |
|
Thanks, looks good! |
Added clean subcommand in reference to issue #80