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

Switch defaults configuration file #2930

Closed
wants to merge 3 commits into from
Closed

Conversation

dra27
Copy link
Member

@dra27 dra27 commented May 10, 2017

Please note at this stage that I'm not interested in any discussion/opinion regarding the variables added in 37d477a, as that whole bit is still being separately worked on (TL;DR I expect to replace them with a single triple/quartet variable)!

However, I'm extremely keen to hear opinions on the rest of this! This GPR adds a new format OpamFile.SwitchDefaults which allows the following kind of file to be set-up:

opam-version: "2.0"
switch-variables: [
  [
    switch-arch
    "%{sys-ocaml-arch}%"
    "Switch architecture (taken from system OCaml compiler)"
  ] {ocaml-system:installed}
  [
    switch-cc
    "%{sys-ocaml-cc}%"
    "Switch C compiler type (taken from system OCaml compiler)"
  ] {ocaml-system:installed}
  [
    switch-libc
    "%{sys-ocaml-libc}%"
    "Switch C runtime flavour (taken from system OCaml compiler)"
  ] {ocaml-system:installed}
  [switch-arch "%{arch}%" "Switch architecture"] {!ocaml-system:installed}
  [switch-cc "cc" "Switch C compiler type"] {!ocaml-system:installed}
  [switch-libc "msvc" "Switch C runtime flavour"]
    {!ocaml-system:installed & os = "win32"}
  [switch-libc "libc" "Switch C runtime flavour"]
    {!ocaml-system:installed & os != "win32"}
]

This list of variables is then filtered at opam switch create and added to the switch configuration as normal. The default switch configuration can be embedded into /etc/opamrc as a switch-defaults section and an example of this is included in 37d477a. There are two areas (in addition to general comments on how this is working) where I'd appreciate some further input:

Locations to search for switch initialisation files

So far, we have ~/.opamrc and /etc/opamrc (as well as the ability to specify a file for opam init to use). I am therefore wondering if the opam init process should write the switch-defaults file (e.g. to ~/.opam/opam-init/switch-defaults.config) or whether we should just have a new chain of files (so look for ~/.opam/opam-init/switch-defaults.config, then the switch-defaults section of ~/.opamrc, then the switch-defaults section of /etc/opamrc).

I was also wondering whether we should be considering having switch-defaults files in repositories - the variables set-up here are to with opam being used for OCaml - should really be homed in the default repository? Indeed, it makes me wonder whether the proper home for sys-ocaml-version, etc. is really a configuration file in opam-repository, and not in OpamInitDefaults at all? Doing this also brings up the important question of how these configuration should be merged or overridden. But it has the nice property that if, say, you have an opam set-up mixing OCaml switches and Coq switches that the action of declaring a new switch with the Coq repository would automatically set that switch up with the switch global variables which a Coq installation might expect (but this may need to worry about merging the switch-variables lists of both Coq and OCaml).

Ability to override switch-variables at opam switch create

In addition to being able to specify a switch-defaults configuration file at opam switch create, it should clearly also be possible to specify values of particular variables at opam switch create (effectively providing a mechanism for opam config set). I would propose adding a further field to switch-defaults:

switch-variables-validation: [
  [switch-arch "x86_64|i686|other" "Sets the host architecture for this switch ($(b,x86_64), $(b,i686) or $(b,other))"] # Clearly not the actual validation...
  [switch-cc "cc|cl" "Sets the C compiler type for this switch ($(b,cc): POSIX C, or $(b,cl): Microsoft C)"]
  [switch-libc "libc|msvc" "Sets the C runtime flavour for this switch ($(b,libc): POSIX C, or $(b,msvc): Microsoft C runtime)"]
]

opam switch create would use this file to generate --conf-switch-arch, --conf-switch-cc and --conf-switch-libc including both validation of the values which can be given and also help text. For my Windows work, this then provides a truly generic way to have opam switch create 4.04.1-msvc32 --packages=ocaml --conf-switch-arch=i686 (rather than my earlier prototypes where --arch, --cc and --libc were hard-coded options)

@dbuenzli
Copy link
Contributor

I was also wondering whether we should be considering having switch-defaults files in repositories

This was my impression aswell, see this discussion.

To sum up I would have liked to have named sets of package constraints and variable definitions that are opam managed and either created locally or distributed via repositories or read from an .opaminit file. These named set can be used for switch init (opam switch create NAME SETNAME) or applied at any time to try to bring a switch to a state w.r.t. a subset of packages.

This could simplify this whole switch init business while at the same time providing a solution to the problem of scaling switch management to local repos: sharing team setups via repositories, having the dev tools you'll need in all your switches under a dev-tools name to apply, snapshoting the constraints of a subset of your current switch under a name to be able to easily apply them to other switches.

I think that with such a scheme you could even eschew the redefinitions via --conf-switch-$(VAR), the opam repo would simply provide all the combination of variables that make sense and make them available under sensible names (e.g. the triplets you mention).

@AltGr
Copy link
Member

AltGr commented May 10, 2017

Thanks for the work on switch-init files, we do indeed need them :)

There are a few things I am not yet sure about:

  1. having the switch-variables depend on package variables, i.e. on the state of the switch, is bugging me: it doesn't match with the current semantics, and is giving a special status to compiler packages again, which I have been trying to avoid (and which was causing problems in an earlier 1.3 prototype). For example, this loses the possibility of creating switches empty by default, and choosing the compiler lazily later on, which was discussed as a possible future feature in another issue.I am not sure what a better solution could be though, we don't have at the moment a way to select both switch-variables and initial packages consistently.

  2. for variables, I would use something more generic (that would pass more easily through cmdliner, the proposal would imply parsing the arguments in multiple passes), for example --set switch-arch=i686,switch-cc=cc. Do we really need the switch-variables-validation ? Or couldn't it be done by the compiler package, since it's related ? A possibility that could be an idea for 1. as well would be to select the compiler based on the specified variables, instead of the converse.

  3. re. specifying switch-init details in the repository. Something close to this, pre-configured switches, was earlier advocated by @dbuenzli, but I can't find the discussion [EDIT: @dbuenzli was quicker, see post above!]. While the configuration can indeed be quite closely related to the repository contents, my intuition is that this would create more problems than it solves. For example, the behaviour when using multiple repositories seems difficult to define well; also, the switch creation options can already include multiple repositories. I am indeed concerned that we use a configuration file that is static while the repository can evolve (we could remain tied by what is defined in the default config at the time of release); but updating the config file won't update any existing switches anyway, so the problem may lie elsewhere.

Besides these points, I think the chain of config files is a good idea, and the proposed format is OK. Other parameters of switch initialisation that could be included in the file include:

  • the default compiler choice (could actually be moved from the global config file) -- or empty ?
  • the choice of repositories (more difficult since this is related to a separate global conf: see the repositories option of opam switch create; but that fails if a repository by this name exists with a different URL)

An idea was to allow that file in source dirs too, so that it could select appropriate parameters for opam switch create srcdir/.

@AltGr
Copy link
Member

AltGr commented May 10, 2017

Note: I recently added the ability to do opam init --config URL, and this could be allowed for switch configs too, making them easier to share.

@samoht
Copy link
Member

samoht commented May 10, 2017

Just a quick note: would it make sense to have simple "scripts" to setup the switches, instead of yet-an-other adhoc format?

Eg. a simple file with:

var set foo $FOO
var set bar $BAR
install ocaml.4.0.4
install foo-x

(where each line is understood as an opam ... command)

@dra27
Copy link
Member Author

dra27 commented May 10, 2017

@AltGr - The switch-variables don't really depend on package variables. The only package variable available is installed which is corresponding to whether the package is included in the --packages list (or has been inferred from the default expression). There's also no dependency after it's been evaluated - it's only being used at switch creation.

You can definitely still set-up an empty switch here - on my x64 system with an x86 OCaml system compiler, it just means that I must run opam switch create --empty foo && opam config set switch-arch i686 && opam install ocaml-system or I will get a complaint because the default for switch-arch will have is x86_64 unless you install ocaml-system at switch creation.

@dra27
Copy link
Member Author

dra27 commented May 10, 2017

@dbuenzli, @AltGr - in principle, the idea of including packages for each possible set of switch variables is a solution, but it results in a vast explosion of the number of compiler packages which would mean the ocaml packages would definitely not be maintainable by hand - for Windows alone, it adds 6 packages for each version, quadrupled once you take the safe-string and flambda variants into account (and exponentially as you add spacetime, etc.). It might also mean that opam-repository suddenly has to be aware of other exotic architectures (in order to have packages for them), rather than just having default cases in configure scripts. I'm rather seeing this feature as way of reducing the number of package variants - e.g. no longer requiring +flambda, +32bit, +spacetime, +safe-string etc.

If these variables (or the triple, etc.) end up being used, my feeling is that they should have some kind of easily discoverable documentation. While nice and generic, --set doesn't give any details of what those variables might be. Validating the values in the compiler package is fine, except that it doesn't create a particularly wonderful user-experience if you make a mistake. Failed switch creation with package rollbacks, etc. is much scarier than being told that you mistyped the value for a command line parameter. I don't feel ultra-strongly about this (it's not a lot code, though - prior to your comments coming in, I was actually just working on it!)

@dra27
Copy link
Member Author

dra27 commented May 10, 2017

@AltGr - the only problem with determining the compiler package from the variables (rather than the other way around) is that are various configurations which would make discovering the ocaml-system compiler harder. Again, on my x64 machine you would always have to do --set switch-arch=i686 in order to get ocaml-system - so it would create a stronger chance that the default opam init builds a compiler, even though there's a system one available.

@dra27
Copy link
Member Author

dra27 commented May 10, 2017

@AltGr - I think the behaviour for having multiple repository with configuration files is entirely related to chaining them already so even though I don't necessarily think we have to have repository-based configuration files, I don't think this problem is simplified just by not permitting them. For example, if I define a single entry in eval-variables in my .opamrc, I think it's the case I automatically override all the eval-variables in /etc/opamrc (and all the defaults)? Especially for configuration items which are lists, I think we may need to think about having options which control how those lists are merged (i.e. an ability to erase an item, an ability to override them, etc.). Mechanisms like that seem worth considering now while the format is being updated, even if we elect not to permit them in repositories yet, I think?

@dra27
Copy link
Member Author

dra27 commented May 10, 2017

@samoht - I don't think it's a bad idea, though it does have an initial feeling of replacing one ad hoc format with another! The idea with the switch-defaults file was that the fields are at least consistent with those in opamrc format (and @AltGr is principally suggesting moving additional fields from .opamrc to this file)

@dra27 dra27 force-pushed the switch-defaults branch from 36a7b38 to 559640e Compare May 10, 2017 16:14
@dra27 dra27 changed the title [WIP] Switch defaults configuration file Switch defaults configuration file May 10, 2017
@dra27 dra27 force-pushed the switch-defaults branch from 559640e to 5fdf375 Compare May 10, 2017 17:08
@dra27
Copy link
Member Author

dra27 commented May 10, 2017

OK, assuming this passes CI - I have added (I think correctly) --config and --default-config options to the opam switch command which mean that you can now pass a switch-defaults file to opam switch create or have opam read the switch-defaults section of either /etc/opamrc or ~/.opamrc.

@AltGr - how's this looking so far? I think that command line overriding (or not) of variables and more complex schemes for merging/locating switch configuration files can be further work on top of this?

@dra27 dra27 force-pushed the switch-defaults branch from 5fdf375 to 416a566 Compare May 11, 2017 08:30
@dra27
Copy link
Member Author

dra27 commented May 11, 2017

It's not clear to me whether the Travis failures are down to this PR or something else?

@AltGr
Copy link
Member

AltGr commented May 11, 2017

Yes, I've read the code and it looks fine (just a note on formatting: I don't have a strict policy on 80 cols, although I try to stick to that, but please avoid very long lines ☺).

About using ocaml-system:installed: indeed, in this case, we don't really depend on a package variable, and semantically this may be sound. I am still bugged by the fact that this is not the normal semantics for the variable ocaml-system:installed, which would normally not be allowed at the switch level. I'll think some more about it, but I think we won't find a satisfactory solution unless there is a way to define both the compiler packages and the associated variables in a common place. That place can't be the package itself (they are at a lower layer), but I am not sure if the config file is the right place either. And something expressive enough to avoid listing an exponential number of variants would be nice, so that excludes one file per compiler...

Yes, a finer way to merge config files would be nice to have. I could see three options: whole-file replace, per-field replace (the current setting), and per-field append on list fields (replace on others). You could already simulate whole-file replace by explicitely disabling built-in and standard conf files;
per-field append would be nice, but wouldn't we still need a way to override the whole field ? I am not sure where the mode should be selected if we have multiple ones: different command-line options (--config, --append-config ?); in-file mode selection ? Possibly per field ?
Enabling to rewrite or delete individual items in the existing config files sounds much too complicated: if I needed something this expressive, I would probably merge the files by hand, and use the new file in whole-file replace mode (if you really need to automate the modifications, you could leverage the new opam-ed tool, actually ☺).

@samoht: the general format is well-defined and consistent with the rest, and has the benefit of allowing our expressive syntax for filters, for example. If you want to avoid anything adhoc, you could still resort to a shell script indeed, which would actually be quite close to what you propose ;)
but actually having an interactive prompt mode and associated script language for opam could be an idea. It could e.g. avoid reloading the whole state between operations. That would be a lot of work though.

@AltGr
Copy link
Member

AltGr commented May 11, 2017

@dra27 there are failures on #2929, which just changes the README, so...

EDIT: apparently, opam-rt doesn't build because of an issue with cohttp.lwt ?

@dra27 dra27 force-pushed the switch-defaults branch from 416a566 to 3d73f4b Compare May 12, 2017 15:32
@dra27
Copy link
Member Author

dra27 commented May 12, 2017

There were some heinously long lines in there, sorry! I've pushed amended commits which I think limit lines to a maximum of 90 chars (the odd function definition I think left as-is). But the stupidly long functionals are now broken sensibly!

Another thought I had on the naming of the ocaml-system:installed was perhaps to change it to ocaml-system:installing but one could imagine in future having the facility to have the switch global variables updated, at which point it would truly mean ocaml-system:installed. Perhaps, given that this variable only exists here (i.e. it's not a true package variable at all, it's just a way of naming the intention to install a package), ocaml-system:selected might look sufficient different (but make sense both at switch creation and also in a hypothetical future "switch update").

For merging the files, what I was thinking was something along the lines of fields used in things like JSON-schema. So having some kind of field which listed changes which should happen to the "parent" file. It's not that I necessarily think it is useful right now, but a major upgrade to the format seems the time to consider changes like that (it's outside the scope of this PR for now, though - perhaps I'll bring it up again for +beta99 😉). It's a fine balance between adding so much configurable power that it's next to impossible to use (e.g. httpd.conf....) vs not enough so that you end up needing to write wrapper scripts around it.

@dra27 dra27 force-pushed the switch-defaults branch 3 times, most recently from 8c75563 to 2ad5da8 Compare May 17, 2017 08:51
@dra27 dra27 force-pushed the switch-defaults branch 3 times, most recently from 1d0b722 to c548064 Compare May 29, 2017 12:15
@dra27
Copy link
Member Author

dra27 commented May 29, 2017

@AltGr - please could you restart the AppVeyor build (I'm pretty sure that's a transient error)

@AltGr
Copy link
Member

AltGr commented Jun 1, 2017

The error seems to be persistent :/ ??

@dra27
Copy link
Member Author

dra27 commented Jun 1, 2017

It's passed now?!

@dra27 dra27 force-pushed the switch-defaults branch from c548064 to 2e81a27 Compare June 3, 2017 10:03
dra27 added 3 commits June 19, 2019 16:46
Add a new format for switch-specific global variables along the lines of
the the "global-variables" field in /etc/opamrc. This new field
"switch-variables" is added to a configuration type "switch-defaults"
which may be embedded as a section in /etc/opamrc.

The motiviation here is to be able to specific switch-specific global
variables which can be fed to opam switch create either by the command
line or by specifying an alternate configuration file.

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Default switch configuration (empty, at present)

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
opam switch create now also supports the --default-config and --config
also found in opam init. For opam switch create, --config specifies a
switch-defaults file, not an opamrc, but if not specified, opam will
look for a switch-defaults section in opamrc.

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
@dra27 dra27 force-pushed the switch-defaults branch from 6c90407 to 52ddeae Compare June 19, 2019 15:47
@XVilka
Copy link
Contributor

XVilka commented Nov 28, 2019

Was there any decision on this pull request? Two years passed already...

@AltGr AltGr added the PR: OUTDATED This PR might still have meaningful content, but is getting stale and might be closed label Mar 11, 2020
@dra27 dra27 closed this Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: OUTDATED This PR might still have meaningful content, but is getting stale and might be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants