Ability to tell jbuilder where to place build artifacts.

[jordwalke]

We're building a project workflow, and many build/cache optimizations are opened up if we are able to tell jbuilder, where to place the artifacts (as opposed to having it be hardcoded to the _build directory). I'm curious how difficult this feature would be to add.
The feature would not merely allow customization of the name of the build directory _build vs. _jbuilder etc, but would allow passing of an arbitrary location on the file system. We could then invoke jbuilder like jbuilder build --buildDest=/myPathTo/buildDir. I believe Crate has a similar feature fwiw.

[rgrinberg]

--buildDest would specify the build root right?

I think that this feature would be easy to add. The difficulty I see here is convincing the maintainers that this feature is desirable.

[jordwalke]

@rgrinberg I'm not sure what you mean by "build root". The requested feature is that --buildDest would determine where jbuilder places the "root of the build directory" which is currently always at _build.

To make things simple, you could require that all invocations of jbuilder supply the same --buildDest or do not supply one at all (in which case --buildDest would default to ./_build).

[jordwalke]

To explain some more about why this is a really compelling feature for us: We're building a tool that allows packages to mark themselves as supporting out of source builds, and ones that do get to share original source files, and build artifacts across many/all projects on the system (in addition to sharing them across the network which will be supported anyways). If jbuilder supports this then projects that use jbuilder will get a significant speedup in our workflow. We want to showcase how fast building multiple projects can be, and jbuilder with out-of-source builds support would make a great demonstration of this. @rgrinberg what would you imagine an objection would be?
I can imagine file watching might become more difficult, but I would absolutely be willing to give up file watching for this feature.

[ghost]

This seems like a reasonable feature to support. I would use --build-dir for the option name though.

Implementation-wise, all we need to do is change Path.to_string and Path.reach to take into account the selected build directory. Though to make sure we catch all cases, we should generalize the use of the Path module. There are still a few cases in the code where we access _build directly. So I suggest to do things in this order:

• move Path to stdune (@rgrinberg already suggested this)
• use Path.t instead of strings for all filenames in Io, Process, etc... and do the conversion to a string as late as possible, i.e. just before call to the stdlib/unix functions
• add a global variable for the build directory and use it for Path.to_string/Path.reach

[rgrinberg]

I think being able to override this in the environment is quite important for this feature. As Jordan would really like to avoid hacking people's jbuilder instructions. Should we use the cmdliner feature that lets us set an option using an env var this?

…

On Wed, Mar 21, 2018, 4:46 AM Jérémie Dimino ***@***.***> wrote: This seems like a reasonable feature to support. I would use --build-dir for the option name though. Implementation-wise, all we need to do is change Path.to_string and Path.reach to take into account the selected build directory. Though to make sure we catch all cases, we should generalize the use of the Path module. There are still a few cases in the code where we access _build directly. So I suggest to do things in this order: - move Path to stdune ***@***.*** <https://github.com/rgrinberg> already suggested this) - use Path.t instead of strings for all filenames in Io, Process, etc... and do the conversion to a string as late as possible, i.e. just before call to the stdlib/unix functions - add a global variable for the build directory and use it for Path.to_string/Path.reach — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#291 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-275-ItgBl3mPPBftMrndiybbleDks5tgWrAgaJpZM4QB1bN> .

[jordwalke]

Yeah, Rudi is right that for this feature, the important part is being able to set this automatically by the environment so that tooling can make sure that all packages build into the correct location without having to make every package owner of every dependency update their build config.

[ghost]

Yep, using this feature of cmdliner seems good

[rgrinberg]

Okay, I'll try and to tackle this. It's easy enough but we do need to decide how to untangle some deps.

For example, Path relies on s-expressions to throw errors. I think it's a good pattern, so I propose moving it to stdune. This will also open the way for us to add sexp converters directly to stdune modules.

We just still need to decide what to do with Code_error. I think we can just change the meaning of this exception to be a "general programming error" rather than a jbuilder specific programming error. Then we can move it to Stdune.Exn (or Stdune.Sexp?).

[rgrinberg]

First step in #716

[rgrinberg]

@diml one property that might be interesting to enforce is to make sure that global variable for build root isn't set after it was already accessed. I'm thinking of something like:

let build_root = ref None

let set_build_root br =
  assert (!build_root = None);
  build_root := br

let build_root () =
  match !build_root with
  | None -> build_root := Some ""; ""
  | Some d -> d

This is to address the possible inconsistencies that might arise from more than 1 build_root being visible to the the rest of the code. The chances of this are slim, but perhaps this extra safety is useful?

The only issue that I see with this is that we'll need to make all those constant paths that are present around the codebase to be lazy about initializing themselves.

But perhaps it would be useful to have a Path submodule like so:

module Static : sig
  val aliases : t Lazy.t
  val db : t Lazy.t
  val log : t Lazy.t
  ...
end

[ghost]

Agreed, but I think build_root should just raise rather than set the root to "". this will make it easier to track places that try to access it before it is set.

For Static, it seems to me that it should go in dune rather that stdune. I would just use unit -> t rather than t Lazy.t.

[jordwalke]

This is a great feature, thank you!

[rgrinberg]

Fixed in master.