Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROPOSAL] Env Defaults #1073

Open
rgrinberg opened this issue Jul 31, 2018 · 18 comments
Open

[PROPOSAL] Env Defaults #1073

rgrinberg opened this issue Jul 31, 2018 · 18 comments
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected

Comments

@rgrinberg
Copy link
Member

rgrinberg commented Jul 31, 2018

Problem

This proposal is coming out of my general annoyance of having to work harder than necessary to allow to customization of the same option at the stanza and the environment level. Clearly, both mechanisms are useful (see flags for example), and ideally this mechanism would work for pretty much every option as well: no_dynlink, preprocess, modes, etc.

Solution

Allow for defining all optional fields inside inside the env stanza. For example:

(env
 (_
  (libraries foo bar)
  (no_dynlink)))

To specify the defaults for a particular stanza, we can:

(env
 (_
  (test
   (enabled_if (= %{foo} foo)))))

Where the stanza could be test, executable, library, alias, and possibly things like buildable as well. We could also entertain setting defaults for multiple stanzas at once but that isn't necessary for a first version.

Implementation

This is the most difficult part, and I haven't yet thought about this too much in the hopes that others might suggest productive ideas. But here are some points that I'd like to mention:

  • Easiest way to implement this feature would be the parse the defaults when parsing the stanzas and inserting them into the correct ~default args. But this is also wrong and will make implementing polling mode harder.

  • All fields that have a default value today will basically need to be options. This is unfortunate as we'll need a layer of indirection to access the values of things once the default values have been set. I wonder if we could save ourselves from this massive refactoring by parameterizing the types appropriately - by using a functor for example.

  • The interaction of :standard with the default value. Should we make it official that :standard will henceforth refer to the current default. Perhaps it should be renamed to :default then? (with a deprecation cycle of course).

@jberdine
Copy link
Contributor

Just a small data point in favor of this proposal: I've run into the need to fall back to OCaml-syntax dune files due to a preprocessor needing different flags in different profiles (e.g. dev vs release). If all the fields could be set in env, then (at least some times) no more need for OCaml-syntax dune files.

@rgrinberg rgrinberg mentioned this issue Aug 1, 2018
4 tasks
@ghost
Copy link

ghost commented Aug 1, 2018

I don't think we should allow any field in env. env is meant for properties that can be changed independently of the source code in order to control code generation or warnings. This is clearly not true for libraries for instance.

It makes sense to put things such as compilation flags, preprocessing flags, or even list of preprocessors in env, given that some preprocessors are just used to instrument the code, but for the rest it doesn't make sense to me.

@rgrinberg
Copy link
Member Author

I don't feel so strongly about libraries as I don't see the use case there either. Also, it's clear that this feature will certainly not work for all fields, so there's no harm in excluding things that we see little use in setting the default for.

But what about things like enabled_if, dynlink, modes, the upcoming no_cross_module_inlining, mode? I see that being able to set all of these in one go as very useful.

@ghost
Copy link

ghost commented Aug 1, 2018

For no_cross_module_inline, definitely, in fact I don't see why it should go anywhere else that env. For no_dynlink and modes, I'm not sure, why is that? These are only useful for very special cases, and additionally they control what targets are generated, so it seems better to me if this information is attached to individual elements. Otherwise it's going to make projects harder to understand.

For enabled_if I guess, however if we systematically write (env (_ ...)) it feels like the syntax of env is a bit wrong.

@rgrinberg
Copy link
Member Author

For no_cross_module_inline, definitely, in fact I don't see why it should go anywhere else that env

This will make it so that users cannot enable this optimization for a single library if there are other stanzas defined in the dir. Obviously this isn't a huge deal, they can just shuffle their directories around to make it work. But the fact that you have to do that, and also the fact that the user can't really gain intuition about what is customizable via env or a stanza is concerning to me. I agree that a project might overuse env and make it difficult to understand what is going on, but I still don't see it as a good enough reason to create sharp edges in the UI for everyone else.

For enabled_if I guess, however if we systematically write (env (_ ...)) it feels like the syntax of env is a bit wrong.

Yeah, this also bothers me but we should be able to fix it once we have the better conditional syntax. In which case, the profile matching form of env won't be necessary.

@ghost
Copy link

ghost commented Aug 1, 2018

Ok, agreed.

Regarding this feature in general, maybe it would help to formalize things a bit more. For instance what is a field, value etc... and then have a general notion of setting the default value of a field and a generic notion for composing fields with their default value, when it makes sense.

@rgrinberg rgrinberg added the proposal RFC's that are awaiting discussion to be accepted or rejected label Oct 22, 2018
@ejgallego
Copy link
Collaborator

ejgallego commented Nov 5, 2018

A common request in Coq is to have a fast build mode which means a byte-only build. Would that be possible to implement with this proposal?

ejgallego added a commit to ejgallego/coq that referenced this issue Nov 5, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ejgallego Emilio Jesús Gallego Arias
In coq#8900 a quicker build target is requested, this PR does provide
such a target for the set of files that I think make sense for
developers.

I think that this approach is simple and effective enough for now.

Dune does indeed support a "build everything" mode that would indeed
be an alternative to this PR, but we are blocked on a [planned]
improvement to Dune:

- profiles should support proper constraining to byte only mode.
- coq_dune should be aware of the "quick" mode as to:
  + not generate build rules for .vo files,
  + use `coqtop.byte` instead of `coqtop`.

The changes to `coq_dune` are trivial so we should follow the Dune
issue upstream ocaml/dune#1073 and maybe
move to this approach.
@rgrinberg
Copy link
Member Author

In conjunction of a workspace file, it would be possible. But I don't think it would be the best way to achieve your goal. Can't you just collect all the bytecode executables you need under a @fast alias? That shouldn't be too hard and will work right away.

@ejgallego
Copy link
Collaborator

ejgallego commented Nov 5, 2018

That shouldn't be too hard and will work right away.

Indeed that's what we are doing now; just wondering what would the best way be. Keep in mind that Coq pervasively uses plugins so collecting .cma files is needed too. And it is a bit painful to do by hand, as there are so many plugins and we risk losing sync.

And indeed, it is very common to compose the main Coq build with some 3rd party plugin, in this case the @fast alias becomes less convenient too.

@rgrinberg
Copy link
Member Author

Keep in mind that Coq pervasively uses plugins so collecting .cma files is needed too. And it is a bit painful to do by hand, as there are so many plugins and we risk losing sync.

I see. So you'd like to rely on the plugins being present in @install to build the .cma files? While that works, that also seems a bit fragile to me. What would you do if you had a non public plugin for example.

And indeed, it is very common to compose the main Coq build with some 3rd party plugin, in this case the @fast alias becomes less convenient too.

Yeah, that's a good point. Although note @fast in this way would likely build too much stuff. It might not be necessary to link all those executables for example.

What is your workflow when working with a bytecode coq? Do you use it interactively and load some plugins? I wonder if it would make sense to bake something coq specific in a plugin to make this truly work well.

@ejgallego
Copy link
Collaborator

What would you do if you had a non public plugin for example.

It should be tested by whomever is depending on it right? Otherwise a non-public plugin wouldn't make a lot of sense I think.

What is your workflow when working with a bytecode coq? Do you use it interactively and load some plugins? I wonder if it would make sense to bake something coq specific in a plugin to make this truly work well.

Bytecode Coq has several uses; in this case we just want to build it as ocamlc is faster than ocamlop and devs want to build the whole ml fileset to check things are OK.

Main use of bytecode Coq these days is for debugging purposes indeed [Drop. command will send you to an OCaml toplevel]

ejgallego added a commit to ejgallego/coq that referenced this issue Nov 5, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ejgallego Emilio Jesús Gallego Arias
In coq#8900 a quicker build target is requested, this PR does provide
targets towards that goal.

- `check`, introduced in Dune 1.5.0, will build all ml files in a fast way,
- `quick`, does build a set of hand-picked targets in a fast way.

Further improvements could come from having `coq_dune` use
`coqtop.byte` when appropriate, taking advantage from
ocaml/dune#1073, and not generating rules
for `.vo` files.
@rgrinberg
Copy link
Member Author

It should be tested by whomever is depending on it right? Otherwise a non-public plugin wouldn't make a lot of sense I think.

Well, private libraries and private executables have found their uses. This might not translate to coq however.

Bytecode Coq has several uses; in this case we just want to build it as ocamlc is faster than ocamlop and devs want to build the whole ml fileset to check things are OK.

For this there's already the @check target. It would not build the cma's however.

This is all unrelated to this thread, but the root of the problem seems to be defining and maintaining aliases is too troublesome for users. It's a bit similar to the issue of defining many tests that all follow the same pattern.

Setting modes in env will indeed be supported!

ejgallego added a commit to ejgallego/coq that referenced this issue Nov 5, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ejgallego Emilio Jesús Gallego Arias
In coq#8900 a quicker build target is requested, this PR does provide
targets towards that goal.

- `check`, introduced in Dune 1.5.0, will build all ml files in a fast way,
- `quick`, does build a set of hand-picked targets in a fast way.

Further improvements could come from having `coq_dune` use
`coqtop.byte` when appropriate, taking advantage from
ocaml/dune#1073, and not generating rules
for `.vo` files.
ejgallego added a commit to ejgallego/coq that referenced this issue Nov 5, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ejgallego Emilio Jesús Gallego Arias
In coq#8900 a quicker build target is requested, this PR does provide
targets towards that goal.

- `check`, introduced in Dune 1.5.0, will build all ml files in a fast way,
- `quick{byte,opt}`, does build a set of relevant hand-picked targets.

Further improvements could come from having `coq_dune` use
`coqtop.byte` when appropriate, taking advantage from
ocaml/dune#1073, and not generating rules
for `.vo` files.
ejgallego added a commit to ejgallego/coq that referenced this issue Nov 5, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ejgallego Emilio Jesús Gallego Arias
In coq#8900 a quicker build target is requested, this PR does provide
targets towards that goal.

- `check`, introduced in Dune 1.5.0, will build all ml files in a fast way,
- `quick{byte,opt}`, does build a set of relevant hand-picked targets.

Further improvements could come from having `coq_dune` use
`coqtop.byte` when appropriate, taking advantage from
ocaml/dune#1073, and not generating rules
for `.vo` files.
ejgallego added a commit to ejgallego/coq that referenced this issue Nov 6, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
ejgallego Emilio Jesús Gallego Arias
In coq#8900 a quicker build target is requested, this PR does provide
targets towards that goal.

- `check`, introduced in Dune 1.5.0, will build all ml files in a fast way,
- `quick{byte,opt}`, does build a set of relevant hand-picked targets.

Further improvements could come from having `coq_dune` use
`coqtop.byte` when appropriate, taking advantage from
ocaml/dune#1073, and not generating rules
for `.vo` files.
@rgrinberg
Copy link
Member Author

Discussing this with the other maintainers, we find that this feature doesn't have so many use cases after all. It should not be used for preprocessors, libraries, and is really only useful for static configuration options such as:

  • modes
  • enabled_if
  • dynlink

@diml does that sound right?

@ejgallego
Copy link
Collaborator

@rgrinberg maybe you could consider adding flags to that list; even if profiles tend to modify flags, it seems to me that they tend to share a few sometimes.

@rgrinberg
Copy link
Member Author

Flags are already definable via the env actually. We might have forgotten to generalize this to a few fields (such as jsoo), but those should be reported separately.

@rgrinberg
Copy link
Member Author

rgrinberg commented Dec 17, 2018

In fact, we should think about how this will work for plugins. For example, we'd like the coq plugin to be able to reuse this functionality for its own flags for example.

@ejgallego
Copy link
Collaborator

Ah sorry, I got confused; I was thinking of inheritance, not of (env ...) definitions.

I meant:

(env
  (foo (flags :standard -w+3 -w+4))
  (_ (flags : standard -w+3)))

vs

(env
  (foo (flags :standard -w+4))
  (_ (flags : standard -w+3)))

@ghost
Copy link

ghost commented Dec 18, 2018

@rgrinberg yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal RFC's that are awaiting discussion to be accepted or rejected
Projects
None yet
Development

No branches or pull requests

3 participants