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

Use dune to build config.ml #167

Merged
merged 7 commits into from Mar 18, 2019

Conversation

Projects
None yet
8 participants
@TheLortex
Copy link
Contributor

TheLortex commented Feb 20, 2019

The change is the following: to build config.cmxs functoria now symlinks config.ml in the _build directory, then it generates a dune configuration file to be able to build _build/_build/default/config.cmxs. It can also grab a dune.inc file in order to add options to the configuration build (necessary for tests).

Tests have been updated to support the modification. They all pass, and I also tested with some Mirage samples. It might be useful to test it on other setups to be sure it doesn't break anything !

(in the mirage/mirage#969 context)

TheLortex added some commits Feb 20, 2019

Use Dune to build config.ml
To build config.cmxs functoria copies `config.ml` in the `_build`
directory, then it generates a `dune` configuration file to be able to
build `_build/_build/default/config.cmxs`. It can also grab a `dune.inc`
file in order to add options to the configuration build (necessary for
tests). Tests have been updated to support the modification.
@TheLortex

This comment has been minimized.

Copy link
Contributor Author

TheLortex commented Feb 20, 2019

By the way this enables the possibility for config.ml to depend on external libraries but I haven't implemented that yet.

Show resolved Hide resolved app/functoria_app.ml Outdated

@Drup Drup self-assigned this Feb 21, 2019

@Drup

This comment has been minimized.

Copy link
Member

Drup commented Feb 21, 2019

I'll take a look at this when I have time. From a quick look, there are various hacks which I'm not fond of, I wouldn't mind input from the usual dune experts, @rgrinberg and @diml.

@diml

This comment has been minimized.

Copy link

diml commented Feb 25, 2019

Just a few comments after having a quick look at the diff:

  • I can see a few references to _build, which means that things might not work as expected when the build directory is set to something else. esy does this for instance
  • it is usually not recommended to manually pass include directories. Indeed, the compiler might read files in these directories but dune won't know about it, which means that it can break incremental compilation
  • paths such as lib/.test_app.objs/byte are internal details of the build rules that are not documented. In particular, they are not covered by the semantic versioning of dune and we already changed them twice to accommodate various features, so I don't recommend making assumption about these paths

In general, if you are trying to do something that can't be done easily with the current feature set of dune, the best is to open a ticket on github.com/dune describing what you are trying to do so that we can discuss and see what can be done.

@TheLortex

This comment has been minimized.

Copy link
Contributor Author

TheLortex commented Feb 25, 2019

I agree with you on this. I translated to dune what was previously done with ocamlbuild to dune so these hacks are not of my creation.

The goal is to build from a config.ml the config.cmxs file, and then dynlink it with functoria. The way I do it is the following:

  • A _build directory is created, inside there is a generated dune file and config.ml is symlinked.
  • dune build --root _build _build/default/config.cmxs is executed to generate the dynlinkable config.
  • Therefore the dynlinkable file is _build/_build/default/config.cmxs.

The problem is that during tests, functoria has to launch dune inside a build context, as config.ml resides in _build/default/tests/app. In order to build config.cmxs I need to define a build context inside the test build context, which is therefore a bit hacky in order to get things right: the drawback is that inside the inner build context there's no access to outer libraries such as test_app, which forces me to use the include hack.

How could we reduce the complexity in that ?

@rgrinberg

This comment has been minimized.

Copy link
Member

rgrinberg commented Feb 25, 2019

The above sounds like a cross compilation problem to me. You want to generate the config.cmxs in the host context, and then use it to build something in the target context.

@diml

This comment has been minimized.

Copy link

diml commented Feb 25, 2019

Would it be easier to define the tests if we consider that functoria has been previously installed? If yes, then the (package functoria) dependency might be the answer: it gives you an environment in which it looks like functoria has been installed. In particular, you can call dune inside this environment. That's how we write all our integration tests in dune. Combined with cram testing, it's really convenient.

@TheLortex

This comment has been minimized.

Copy link
Contributor Author

TheLortex commented Feb 25, 2019

Thank you for your answers. I've been able to get rid of most hacks using the (package functoria) dependency at the cost of defining a functoria.test_app package.
Now the behavior is the following:

  • app/config.ml and app/dune.inc are symlinked in a app/build-config/ directory.
  • app/_build/default/build-config/config.cmxs is built using dune. In the test case, it's has access to functoria and test_app.
  • dynlinking is done as before.

I'll push the update soon.

EDIT: Tests seems to pass but it induced another bug in Mirage unikernels, I'm working on it.
EDIT2: This is fixed.

@TheLortex TheLortex force-pushed the TheLortex:use-dune branch from e438a7e to 6a228ab Feb 25, 2019

Update config.ml build to reduce complexity
Now `config.ml` and `dune.inc` are copied in their own `build-config`
directory, where `dune` is also generated. Tests don't rely anymore on
"include hacks".

@TheLortex TheLortex force-pushed the TheLortex:use-dune branch from fc60242 to dd94b19 Feb 26, 2019

Show resolved Hide resolved app/functoria_app.ml Outdated
Show resolved Hide resolved app/functoria_app.ml Outdated
"myocamlbuild.ml" (* FIXME: add a .mirage-ignore file to avoid this *) ]
["main.ml"; "app.ml"; "config.dune"; ".mirage.config"; "dune"; "key_gen.ml";
"build-config"; "dune-project"; "_build"
(* FIXME: add a .mirage-ignore file to avoid this *) ]

This comment has been minimized.

@emillon

emillon Feb 28, 2019

Contributor

As a reminder, don't forget this :)

Small updates
- use dune lang 1.0 instead of 1.7 when building config.ml
- reuse the build-config location variable
@Drup

This comment has been minimized.

Copy link
Member

Drup commented Mar 3, 2019

Hmm, is using symlinks a potential issue for portability ?

@TheLortex

This comment has been minimized.

Copy link
Contributor Author

TheLortex commented Mar 4, 2019

@Drup On Windows Unix.symlink is a privileged operation (according to OCaml's Unix module documentation) so that might be an issue. However there is already some code that uses Bos.OS.Path.symlink (https://github.com/mirage/functoria/blob/master/app/functoria_app.ml#L584) therefore we might ignore that for now. I don't see any other portability issue. What do you think ?

@avsm

This comment has been minimized.

Copy link
Member

avsm commented Mar 11, 2019

This looks good enough to merge and work on in master for a while...

@samoht

This comment has been minimized.

Copy link
Member

samoht commented Mar 17, 2019

Just a few remarks:

  • could you use a copy rule in the dune file instead of symlinks?
  • is config.dune mandatory? Can you describe how to use that file to add external dependencies? I think the current norm is to name these files dune.<smting> so dune.config would be better.

Once this is cleared out, I think we should merge this PR to unlock the dune+mirage port.

@rgrinberg

This comment has been minimized.

Copy link
Member

rgrinberg commented Mar 17, 2019

Let me just note that using the symlink action in dune does not cause portability issues. Dune will in fact copy the file anyway on windows.

@TheLortex

This comment has been minimized.

Copy link
Contributor Author

TheLortex commented Mar 18, 2019

@samoht

* could you use a `copy` rule in the `dune` file instead of symlinks?

I encounter some problems with that because dune.config and config.ml can live outside the dune workspace when the -build-dir is option used.

* is `config.dune` mandatory? Can you describe how to use that file to add external dependencies? I think the current norm is to name these files `dune.<smting>` so `dune.config` would be better.

Let's use dune.config then. This file is optional, and if it exists it's included in the generated dune file, and a dependency to a dummy config_custom library is added. To add external dependencies, you just have to specify them in the dummy library specification, inside dune.config. This is done for tests, for example:

(library
    (name config_custom)
    (libraries functoria.test)
    (modules)
)
@samoht

This comment has been minimized.

Copy link
Member

samoht commented Mar 18, 2019

Looks good, thanks!

@samoht samoht merged commit d917a5e into mirage:master Mar 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.