Environment & build profiles

[ghost]

Configuration:

(env
 (release
  (flags (:standard -O3)))
 (foo
  (flags (:standard -w +a))))

usage:

$ jbuilder build --profile foo

[bobot]

What is an Env_node ?

[ghost]

It represent the environment in a given directory. I haven't written the details yet, but the idea an environment is attached to every directory. It is inherit from the parent up to the root of the current scope, where the global default environment is used. (env ...) stanzas are used to modify the environment.

To know the value of a variable, one must look at (env ...) stanzas in the current directory and parent directories up to the root of the current scope. I'm planning to add a jbuilder printenv <dir> command that will print the computed environment for a given directory.

[rgrinberg]

You have the dev_mode function now.

[ghost]

I'm wondering if this feature shouldn't be a bit more generic with just a toplevel set stanza:

(set flags (:standard -O3))

and to do different things according to the profile, we could have generic constructions:

(when (= ${profile} dev)
 (set flags (:standard -w +a)))

(unless (= ${profile} release)
 (set flags (:standard -w +a)))

(case ${profile)
 (dev ...)
 (release ...))

This could subsume #397:

(case ${os}
 (win32 (set backend windows) (set backend_lib foo))
 (_     (set backend other)   (set backend_lib bar)))

(rule (copy# file.${backend}.ml file.ml))

(library
 ((name blah)
  (libraries (a b ${backend_lib})))

[rgrinberg]

I like the more general approach here. How are profile will be different from any other variable then? Just the fact that we can set them in the command line?

[ghost]

Yh, also maybe we'll be able to set other things from the command line. We should probably disallow changing the profile in a dune file as well, otherwise it's going to be difficult to understand what's happening

[rgrinberg]

--dev missing a dash.

[rgrinberg]

Can we select the build profile per context?

[ghost]

Yes, I started a patch for it, I'm not sure I pushed it though

[jberdine]

Let me know at any point if you could use some alpha testing of this. (I tried from this branch, but it seems to have fallen a bit far behind.)

[ghost]

I think what stopped this feature was more a design question. However, it might take quite a lot more time to design and implement something better and not being able to change the defaults is quite annoying. I guess we could start with this PR since it solves the problem. It's simple so it shouldn't be too hard to be backward compatible if we decide to change the system later.

I'll rebase it, some testing would be welcome indeed.

[ghost]

I rebased and finished this PR.

[rgrinberg]

Wondering if this is the result of a rebase accident.

[ghost]

This is correct, basically for the printenv command we need to access the super context. So instead of keeping the stanzas, we just keep the whole super contexts now.

[rgrinberg]

Yeah, that is fine. Just wondering why you brought back the String_map.

…

On Thu, May 3, 2018, 5:53 PM Jérémie Dimino ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/gen_rules.ml <#419 (comment)>: > in Fiber.parallel_map contexts ~f:make_sctx >>| fun l -> let map = String.Map.of_list_exn l in Build_system.set_rule_generators build_system - (String.Map.map map ~f:(fun ((module M : Gen), _) -> M.gen_rules)); - String.Map.iter map ~f:(fun ((module M : Gen), _) -> M.init ()); - String.Map.map map ~f:snd + (String_map.map map ~f:(fun (module M : Gen) -> M.gen_rules)); + String_map.iter map ~f:(fun (module M : Gen) -> M.init ()); + String_map.map map ~f:(fun (module M : Gen) -> M.sctx) This is correct, basically for the printenv command we need to access the super context. So instead of keeping the stanzas, we just keep the whole super contexts now. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#419 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIe-30vHQdB_5VWxBRiEqDHf59dmQiRks5tuuHCgaJpZM4RgOss> .

[ghost]

Ah, I guess it's because there is still a String_map in import.ml so I didn't catch this error. I'm removing it.

[ghost]

Actually it's a bit annoying as it's still used by many pretty-printers.

I added a commit to make sure env doesn't appear more than once in a jbuild file

[jberdine]

Nice, this is working for me. One step closer to being able to use sexp jbuild files!

I don't know if it's difficult or leads down the slippery slope to hard design questions, but I wanted to write:

(env
 (coverage
  (flags (-g -opaque))
  (preprocess (pps (bisect_ppx)))))

in order to eliminate code in jbuild that detects if bisect_ppx is installed.

[jberdine]

Additionally, it would be useful to be able to use :include with env to avoid duplicating this configuration in every subdirectory's build file.

(env
 (:include "${SCOPE_ROOT}/dune-env"))

Or even allow including whole (lists of) stanzas:

(:include "${SCOPE_ROOT}/dune-common")

This is not a big deal, as it's not hard to cat some files together to generate the jbuild files, but direct support would make it tidier.

[ghost]

The environment is inherited from the parent directory, up to the root of the current scope (project). So you should only need to write it once in a toplevel jbuild file.

Regarding preprocess, that seems fine, I was thinking of adding something like this at some point. For the case of bisect in particular, we are also planning to have first class support (#616).