Skip to content
This repository has been archived by the owner on Aug 25, 2022. It is now read-only.

Update names #174

Merged
merged 2 commits into from
Jul 30, 2019
Merged

Update names #174

merged 2 commits into from
Jul 30, 2019

Conversation

samoht
Copy link
Member

@samoht samoht commented Jul 30, 2019

  • Change build-config to simply config to be closer to config.ml.
  • Change custom_config to mirage_config to have both mirage and config in the library name (to reflect the use of that library better).

This reflects config.ml a bit more.
Make sure `mirage` and `config` are in the library name.
@samoht samoht requested a review from TheLortex July 30, 2019 12:47
@samoht
Copy link
Member Author

samoht commented Jul 30, 2019

/cc @hannesm for feedback

  • config/ is now the folder where config.ml is built.
  • mirage-config can be used in dune.config to add extra (local or extra) dependencies to be used in config.ml

Copy link
Member

@TheLortex TheLortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names look better indeed ! Ideally we wouldn't need this additional config/ directory but for now it's hard to do without it.

@samoht samoht merged commit c7f8b2b into mirage:master Jul 30, 2019
@samoht samoht deleted the fix-names branch July 30, 2019 14:42
@hannesm
Copy link
Member

hannesm commented Jul 31, 2019

looks fine to me (sorry for my slow response), re-reading #167 and #171, I have questions:

  • "By the way this enables the possibility for config.ml to depend on external libraries but I haven't implemented that yet" (from Use dune to build config.ml #167) <- is this implemented now? (if so, how would I do this?) <-- to me this is very interesting because it means we could break up this mirage utility which knows way too much about opam packages
  • "The PR currently breaks API with Mirage" (from Replace dynlink method by a 2-stage build #171) <- is this what got merged? (I'm fine with breaking APIs, but we should ensure we get the mirage utility up to date then)
  • IIUC previously config.ml was built in _build, now there's a directory config generated (to put the build files), is this correct? I'd be in favour of prefixing such generated directories with _.

(sorry, I should likely just pin functoria{-runtime} and test it with mirage 3.5.1 ;)

thanks for your (esp. @TheLortex) work on this!

@samoht
Copy link
Member Author

samoht commented Jul 31, 2019

  • yes it's possible to use external libraries in config.ml now: it needs to be added in the (optional) dune.config file. We'll add a few examples in mirage-skeleton. One remaining issue is that opam configure will not install these dependencies automatically so they need to be somehow available when configuring the unikernel. Not sure sure how to fix this properly yet.

  • no API breakage has been merged (Lucas reverted his changes after one of my comments)

  • the config directory contains a symlink to config.ml, a dune file (and optionally a symlink to dune.config) so for me it's more like a source tree, hence its name. dune will build this into _build/default/config. I am almost tempted to allow people to create config/{config.ml,dune} manually in the future, as it's not so complicated to generate the right build rules (and it will allow people to customize these more easily).

@hannesm
Copy link
Member

hannesm commented Jul 31, 2019

that all sounds excellent to me -- yes, I'm in favour of generating less code/symlinks/files (it's always a burden if you want to modify things or get errors in the generated stuff). cool!

@hannesm
Copy link
Member

hannesm commented Jul 31, 2019

with the current state, is the symlink only done if the file does not exist (yet)? (same for dune -- it should imho only be generated if its not present or was previously generated and has not been modified (and has changed) <- that's a bit hard to figure out programmatically, but would allow us to get rid of the dune.config).

@samoht
Copy link
Member Author

samoht commented Jul 31, 2019

Currently mirage configure overwrite any existing config/config.ml which kind of makes sense as mirage clean removes everything anyway. I agree that it's probably be better to not do this in the future, but we will need to fix mirage clean as well.

@samoht
Copy link
Member Author

samoht commented Jul 31, 2019

For using multiple files and using external libraries, you can create a dune.config file with the following contents:

;; copy additional files
(rule (copy# ../foo.ml foo.ml))

(library
  (name mirage_config)
  (wrapped false)
  (modules foo)
  (libraries <external libraries>))

If you don't care about new files, just use:

(library
  (name mirage_config)
  (modules)
  (libraries <external libraries>))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants