Jbuilder exec rebuilt if possible option

[rgrinberg]

This option turns out to be quite handy in practice. Been using it for the last day without major issues. I've elected to just make its own option for now because --dev seems like it would leave the user guessing. Maybe it's best to support both flags?

Fix #317

cc @copy

[rgrinberg]

Right now the only caveat with this PR is the error handling. In some cases, we know the path must exist locally in the build system, so a build error is more appropriate than a not found error.

[ghost]

BTW, I just saw the comments about jbuilder exec building the executable and --dev. I wasn't clear in my first comment about it, I never meant that the building behavior should only be enabled when --dev is passed.

For me the issue is the following, if you type:

$ jbuilder build --dev bin/main.exe
$ jbuilder exec bin/main.exe

then the second line will rebuild the executable from scratch since the compilation flags have changed globally. This is annoying but this is not specific to jbuilder exec. We have the same issue with jbuilder utop for instance. For me the right way to fix this is to do it globally. For instance we could allow to write (dev) in the jbuild-workspace file to enable the development mode once and for all.

Regarding jbuilder exec, I tend to think that it should always build the executable, since this is more convenient. Possibly we could have an option to disable it.

[rgrinberg]

Thanks for clarifying @diml, I understand now. So I think it's just a matter of switch the flag to be --no-build. Unless anyone wants to argue this point, I will do that before soon enough. Comments about the implementation are welcome.

[rgrinberg]

I will also add the --dev option to jbuilder exec to at least give users the option to avoid this issue.

[rgrinberg]

@bobot @diml I've changed the default to build. --dev was already supported btw. But i added a test for it just in case.

[ghost]

I had a quick look at the code, it looks fine to me. resolve_targets'/resolve_targets should be renamed resolve_targets/resolve_targets_exn.

In the test case, I would prefer if the first error was something like: Program ./foo.exe isn't built yet, you need to build it first or remove the --no-build option.

BTW, resolve_targets and the code for resolving executables is becoming a bit complicated. Now that we can give arbitrary requests to the build system, we could encode what this code is doing in the build arrow directly. This doesn't need to be done in this PR though, but we should keep it in mind if it becomes too complicated.

[rgrinberg]

I had a quick look at the code, it looks fine to me. resolve_targets'/resolve_targets should be renamed resolve_targets/resolve_targets_exn.

Done.

In the test case, I would prefer if the first error was something like: Program ./foo.exe isn't built yet, you need to build it first or remove the --no-build option

You only expect this to work for relative paths, right? Because to make it work in general, i.e. for things like jbuilder exec foo would require us to know if foo is installable. Which is possible, but we don't really have a convenient way to do it.

BTW, resolve_targets and the code for resolving executables is becoming a bit complicated. Now that we can give arbitrary requests to the build system, we could encode what this code is doing in the build arrow directly. This doesn't need to be done in this PR though, but we should keep it in mind if it becomes too complicated.

+1

[ghost]

You only expect this to work for relative paths, right? Because to make it work in general, i.e. for things like jbuilder exec foo would require us to know if foo is installable. Which is possible, but we don't really have a convenient way to do it.

Well, we can test if _build/install/default/bin/foo is a target for instance. But we can leave that until we refactor resolve_targets. For instance we could add a function like this Build_system:

(** Returns the first file that the build system knows how to build *)
val find_buildable_target : t -> Path.t list -> Path.t option Future.t

then we'd do calls such as

Build_system.find_buildable_target ["_build/install/default/bin/foo"; "/usr/bin/foo"; ...]

That'd make the code a bit simpler overall I think.

[rgrinberg]

OK, so then there's no more work in this PR to be done if we should do the refactoring to improve the error message. I'll create the relevant ticket for this work on this PR is merged then.

[rgrinberg]

@diml i ended up implementing your error message improvement for all jbuilder exec variations because it was easy enough.

[ghost]

OK, seems good