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

Some notion of "environment" #391

Closed
lpw25 opened this issue Jan 3, 2018 · 11 comments
Closed

Some notion of "environment" #391

lpw25 opened this issue Jan 3, 2018 · 11 comments

Comments

@lpw25
Copy link

lpw25 commented Jan 3, 2018

A few weeks ago I made a suggestion to @rgrinberg about how bisect_ppx and similar problems might be addressed. He suggested that I make an issue containing the suggestion, so that it is at least written down somewhere.

The problem of bisect_ppx is that you want a single place to change a setting across all the libraries in a single project. That does not fit well into jbuilder's current model. In order to make sure that mutiple packages can be built together in a single workspace, jbuilder insists that the jbuild file for a library contains all the details of how to build it -- whereas with bisect_ppx we want to change the build instructions of multiple libraries from a single location.

My suggestion is that, if you want to have builds also depend on some things external from the jbuild files, then you should make those things a first-class notion. So:

  1. We add some explicit notion of "environment" to jbuilder and a way for plugins and the implementations of jbuild constructs to query it for information.

  2. The implementations of things like the library construct can look up things like the default preprocessors and the default warnings from the environment.

  3. Allow the environment to be set using a jbuild.env file containing sexps that define entries in the environment. I suggest that there should be a single jbuild.env for a "scope" and that it should be required to be at the root of the scope (i.e. next to a .opam file). A more relaxed scheme could be supported if there is demand for it.

  4. Now that we have a sexp file representing the environment, just like we have a sexp file representing the build instructions, we can extend the plugin mechanism (see here) to work on these files as well.

With these changes you could write:

(lang jenv 1.0 (bisect 1.0))
(default_warnings (:standard +57))
(bisect:on)

in your jbuild.env to add a default warning and to turn on bisecting. The (bisect:on) part would use a function of type Sexp.t -> Environment.t provided by the bisect plugin to make the required changes to the environment to turn on bisecting.

@rgrinberg
Copy link
Member

Will the environment be able to subsume the concept of profile @diml mentioned here #371 (comment)

It will be necessary to select the profile from the command line, which doesn't seem possible as an env plugin.

@lpw25
Copy link
Author

lpw25 commented Jan 4, 2018

I think you could implement profiles as just a flag in the environment, set on the command-line and with some form of match construct. So like:

(lang jenv 1.0)
(default_warnings
  (match_profile (foo (:standard +a))
                          (_ (:standard))))

so that jbuilder --profile foo would turn on all warnings. Variations on this could allow the match construct to work on any flag in the environment, or could build the match into things like default_warnings so that you were always expected to specify in which profile the setting applied.

@rgrinberg
Copy link
Member

OK, this is nice. I think you can basically take this further and subsume workspace files as well. It's not clear to me what's the logical distinction between them anyway.

I'm wondering if we could just implement what diml proposed for profiles today, and then generalize it with this env idea after we make plugins work. It seems like this will all be fairly compatible.

@lpw25
Copy link
Author

lpw25 commented Jan 4, 2018

I think you can basically take this further and subsume workspace files as well.

I'd be a bit cautious there. I think you need some information in the workspace file before you would be able to load any plugins. So it would be quite difficult to include plugins for workspace files.

It's not clear to me what's the logical distinction between them anyway.

How I'd been thinking of them, I'd been expecting projects to (optionally) include their own environment file, whilst I expect users to mostly write workspace files. The workspace stuff is very specific to the machine your currently building on, but the environment stuff is just useful development settings for a particular project.

Possibly the environment file could replace the jbuild-workspace.dev files, which have always been a bit weird.

@lpw25
Copy link
Author

lpw25 commented Jan 4, 2018

Possibly the environment file could replace the jbuild-workspace.dev files, which have always been a bit weird.

The environment file could also contain the (project ...) stanza that Jeremie wants to simplify the project layout.

@ghost
Copy link

ghost commented Jan 9, 2018

Why do we need separate files? I would expect that users will want to commit the environment files most of the time, and when the settings depend on the local machine, they could just put them in the jbuild-workspace file, a temporary jbuild file or even commit them as a specific profile. As long as the release profile stays free of non-portable stuff, it seems fine to me.

BTW, the term environment might be confused with the unix environment. It seems to me that this mechanism will be used to set the defaults in a given scope, what about calling it defaults?

@lpw25
Copy link
Author

lpw25 commented Jan 9, 2018

Why do we need separate files?

Well, at the moment there are two kinds of file: jbuild files that only affect the current directory and jbuild-workspace files that affect everything underneath the current directory but are intended for local machine settings and are not generally committed into repos. Here we want a file that affects everything underneath the current directory but has general settings that should be committed in the repo.

I think three separate kinds of file might not be a bad idea. One local one for the current workspace, one file to represent the details of a project, and one in each directory describing how the things in the directory are built. This could avoid the current requirement for an opam file to indicate the bounds of the current project. You could even imagine generating the opam file based on the contents of the project file -- although I'm not sure we want to go that far.

I think workspaces could never be written with plugins because they are needed too early in the dependency chain, so that might rule out using them for environments.

An alternative would be to allow jbuild files to contain a project construct that contains the project info, including environment/defaults changes. This would mark the root of the project and would not be allowed to be nested, so it wouldn't let jbuild files affect sub-directories more generally.

the term environment might be confused with the unix environment

That's somewhat deliberate, if we wanted environment variables to affect compilation I would expect them to be made available thought the environment type.

what about calling it defaults

That seems overly specific to me. Defaults are a good use case, but I'm not sure they are the only use. I guess it depends on whether you are talking about the name of the type (Environment.t in my description), the name of the file (jbuild.env in my description), or the name of constructs to change the environment (default_warnings in my description, defaults in your proposal to control defaults from workspace files).

  • For the name of the type I think it should be a general name like Environment.t or Context.t in the future we may want to put all kinds of things in there.
  • For the name of the file it depends on what the file is precisely for -- if the environment file itself is made into a more general description of a project it could just be a project file.
  • For constructs making changes to the environments I think using names based around defaults for making changes to default settings is fine.

@ghost
Copy link

ghost commented Jan 9, 2018

An alternative would be to allow jbuild files to contain a project construct that contains the project info, including environment/defaults changes. This would mark the root of the project and would not be allowed to be nested, so it wouldn't let jbuild files affect sub-directories more generally.

I was thinking of something like this. I was thinking to have different stanzas to define the project and the environment, to allow modifying the environment in a sub-tree of a project.

Delimiting projects with a special file might be good, so introducing project files might be OK anyway. I guess the question here is how we want the environment changes and project scoping to interact. It seems clear that we don't want environment changes to leak between projects. The other question is whether we want to allow changing the environment only in part of a project:

  • if no, then it seems natural to put the environment definition in the project stanza/file
  • if yes, then it seems natural to put the environment definition in dedicated stanzas/files

The latter seems better to me.

You could even imagine generating the opam file based on the contents of the project file -- although I'm not sure we want to go that far.

I definitely want to have that at some point :). I was thinking of generating the opam files using all the information available to jbuilder when the workspace is self-contained. If we only used the project file, then we'd just end up writing the opam files in a different syntax. Users might not necessarily work with closed workspaces on their machine, but at least the CI could check that the opam files are up to date.

I think workspaces could never be written with plugins because they are needed too early in the dependency chain, so that might rule out using them for environments.

Agreed, and I don't see the need to implement build contexts as plugins anyway. But I don't see how the two are connected. It seems to me that there is a question of whether we want to take environment changes into account when building the plugins, but that seems independent to where we put such environment changes.

That's somewhat deliberate, if we wanted environment variables to affect compilation I would expect them to be made available thought the environment type.

Ok, that makes sense to me

@ghost ghost mentioned this issue Jan 11, 2018
@ghost
Copy link

ghost commented Jun 2, 2018

We have something like this in master now

@ghost ghost closed this as completed Jun 2, 2018
@lpw25
Copy link
Author

lpw25 commented Jun 2, 2018

For my curiosity, which PR has the details of the stuff in master. Or failing that where in the source or examples should I look to see how things are shaping up.

@ghost
Copy link

ghost commented Jun 2, 2018

It's #419.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants