solvuu / solvuu-build

DEPRECATED. We recommend Jane Street's dune (formerly jbuilder).
ISC License
24 stars 5 forks source link

More convenient way of passing the same configuration to apps and libs #48

Closed copy closed 7 years ago

copy commented 7 years ago

You can consider this issue a question, but I think some api changes may be necessary.

Currently Solvuu_build.Std.Project.app and Solvuu_build.Std.Project.lib share many parameters, for which the user often wants to pass the same values (e.g., compiler options). It would be more convenient if, for example, a single config parameter would be used.

agarwal commented 7 years ago

I've thought about factoring out the common parameters (everything from annot to warn_error) and defining them in a separate record type, e.g.:

type options = {annot : unit option; bin_annot : unit option; ... ; warn_error : string option}

To make this useful, we'd then have to remove all the individual arguments from the lib and app functions, and replace with a new argument of type options. But now, how does one create an options value? I guess we should provide a function:

val options : ?annot:unit -> ... -> ?warn_error:string -> unit -> options

Now, if you really want the exact same options for both your lib and app, then you'd only have to call this function once, and you could use the result twice. If even 1 value differs, you'd have to call this function twice or do a record update. You haven't saved much typing, so I wasn't convinced it was worth introducing an extra data type for this, which has its own disadvantage of extra mental overhead.

In my projects, I've been doing this:

open Solvuu_build.Std

(* Define all the options that I want to set. *)
let annot = Some ()
let bin_annot = Some ()
...
let w = Some "A-4-33-41-42-44-45-48" 

(* Override definition of [lib]. *)
let lib ~pkg ~dir ~style lib_name =
  lib ?annot ?bin_annot ?w ~pkg ~dir ~style lib_name

(* Override definition of [app]. *)
let app ~file app_name =
  app ?annot ?bin_annot ?w ~file app_name

Let me know your thoughts on these alternatives.

copy commented 7 years ago

I've seen this style being used in projects that use solvuu-build. Unfortunately, it means you still have to specify each parameter twice (once for app and once for lib), which is quite burdensome since we're using more than 10 parameters already.

One alternative (quite similar to what you proposed) would be to let the user define a function that accepts app or lib which then applies all the necessary options to them:

let apply_options fn = fn ~annot:() … ~warn_error:"A-26-27-58"
let myapp = (apply_options Project.app) ~file:"app.ml" "my_project"

Unfortunately, OCaml doesn't seem to accept this without a type annotation on fn. Now, if solvuu-build would provide this type

type 'a with_options =
  ?annot:unit ->
  ?bin_annot:unit ->
  ?color:[ `always | `auto | `never ] ->
  ?g:unit ->
  ?inline:int ->
  ?inlining_report:unit ->
  ?optimize_classic:unit ->
  ?optimize2:unit ->
  ?optimize3:unit ->
  ?remove_unused_arguments:unit ->
  ?safe_string:unit ->
  ?short_paths:unit ->
  ?strict_sequence:unit ->
  ?thread:unit ->
  ?unbox_closures:unit ->
  ?w:string ->
  ?warn_error:string -> 'a

We could write

let apply_options (fn : with_options) = fn ~annot:() … ~warn_error:"A-26-27-58"
let myapp = (apply_options Project.app) ~file:"app.ml" "my_app"
let mylib = (apply_options Project.lib) ~pkg:"lib" ~style:`Basic ~dir:"lib" "my_lib"

But I'm not sure if this is nicer than the specifying a record for all the shared options.

After all, when using a separate record for the options, you can also provide:

val update_options : options -> ?annot:unit -> ... -> ?warn_error:string -> unit -> options

Another alternative would be to merge app and lib to function similar to:

type app_or_lib
val make = ?annot:unit -> … -> ?warn_error:string -> app_or_lib -> item
val app = file:string -> string -> app_or_lib
val lib = pkg:string -> dir:string -> style:[ `Basic | `Pack of string ] -> string -> app_or_lib

Which would change the usage to:

let mymake = make ~annot:() … ~warn_error:"A-26-27-58"
let myapp = mymake (app ~file:"app.ml" "my_app")
let mylib = mymake (lib ~pkg:"lib" ~dir:"lib" ~style:`Basic "my_lib")

Either way works for us, so long as we don't have to write out every parameter twice.

agarwal commented 7 years ago

7e7d3dcfc4bd56bf14218e6577bb9b866688f199 adds a with_options type. This seemed the cleanest to me. It is also useful to factor out the signature declarations of the lib and app functions.

An app_or_lib type seemed messy. That's already what an item is, and having another type that kind of sounds like the same thing wasn't appealing.

Defining a record type containing all the options could be okay too. It could be used to factor out the definitions of the lib and app types. However, that introduces an extra level of indirection for record field access, which I didn't feel adds value.

Please close this issue if you're content with the solution.

copy commented 7 years ago

This is a great solution, cheers.