regressions in functoria 3.0

[hannesm]

I finally found the time to try functoria 3.0.0 out, and noticed some regressions //cc @samoht @TheLortex

• (1) mirage configure -f config2.ml used to work, now I get:

File "config/dune", line 4, characters 11-17:
4 |   (modules config)
               ^^^^^^
Error: Module Config doesn't exist.
error while executing dune build --no-print-directory --root
                        /usr/home/hannes/devel/mirage/mirage-skeleton/device-usage/network/./
                        @config

• (2) a typo/error in config.ml leads to an error location in config/config.ml -- that's unexpected since I never did anything in the config subdirectory (previously the error location was in config.ml)

File "config/config.ml", line 11, characters 0-3:
11 | foo
     ^^^
Error: The function applied to this argument has type [...]

• (3) a unikernel (such as https://git.robur.io/?p=openvpn.git;a=tree;f=mirage-client;h=c69bca6813e17f462ed9373806113afc9a8ccaf5;hb=HEAD) that uses the subdirectory config to crunch the configuration:

  • (a) removed my configuration and .gitignore during mirage clean (that's unexpected and a bit too eager, how will I now get my configuration back?)
  • (b) after mirage configure && mirage build the crunched static1.ml contains more files than expected
    ~> maybe functoria should be a bit more careful to only remove stuff it generated itself? (and/or warn/refuse to do anything if a hand-crafted config directory exists?; or reside in some _YYY directory (as already used by mirage: _build / _build-ukvm / _build-solo5-hvt))

• (4) app-info is broken: a mirage configure && mirage build in mirage-skeleton/tutorial/app_info leads to:

Fatal error: Invalid character in package name "\027[01;04mlwt\027[0m"

run ['opam' 'config' 'subst' 'info_gen.ml']: exited with 99run ['dune' 'exec' '--root'
     '/usr/home/hannes/devel/mirage/mirage-skeleton/tutorial/app_info/./'
     '--' 'config/config.exe' 'build']: exited with 1

• (5) mirage configure -b tst && mirage build -b tst leads to an error:

Error: Path . has already been scanned. Cannot scan it again through symlink
tst
error while executing dune build --no-print-directory --root
                        /usr/home/hannes/devel/mirage/mirage-skeleton/tutorial/app_info/tst
                        @config

[samoht]

Many thanks for the feedback, I'll address these issues next week and I'll add a few tests to avoid this kind of regressions in the future.

[hannesm]

thanks @samoht -- about (2) and (3), esp. taking your comment into account, I was wondering why a config directory is needed? can't we just use the toplevel directory and generate dune/dune-project/.. there? (since we already generate e.g. main.ml etc. in the toplevel directory)

[hannesm]

and for the record, I'm fine with dropping -f and -b (which seem to be rarely used anyways) from functoria to solve (1) and (5)

[samoht]

I think the idea was to generate a dune file in a separate directory to not overwrite any existing dune file (and for instance one which might be created by mirage to build unikernel.ml).

So maybe we should generate a dune.config and dune.unikernel and include these in a top-level dune file. Anwyway, lots of good design questions, I'll think about it a bit more :-)

[samoht]

An other option is to generate main.ml in a separate build directory so the toplevel stay clean.

[hannesm]

@samoht there's for sure a huge design space which files to generate and where to put them. I'd try to minimize the amount of generated files, since it is always a burden on the user, esp. if these are files someone is trained to write by hand in other projects (such as dune).

another regression in the mirage-skeleton build (other warnings are now active and error out), from https://travis-ci.org/mirage/mirage/jobs/567876604

make[1]: Leaving directory '/home/opam/mirage-skeleton/applications/dns'
cd applications/static_website_tls && mirage configure -t unix 
File "config/config.ml", line 7, characters 16-27:
Warning 3: deprecated: Mirage.http_server
`http_server` is deprecated. Please use `cohttp_server` or `httpaf_server` instead.
File "config/config.ml", line 1:
Error: Some fatal warnings were triggered (1 occurrences)
error while executing dune build --no-print-directory --root
                        /home/opam/mirage-skeleton/applications/static_website_tls/./
                        @config

make: Leaving directory '/home/opam/mirage-skeleton'
make: *** [Makefile:49: applications/static_website_tls-configure] Error 1
'unset TESTS; OPAMYES=1 export OPAMYES; set -uex; sh ./.travis-ci.sh' exited 2. Terminating with 2
The command "bash -ex .travis-docker.sh" exited with 2.

[samoht]

See #176 for a first tentative to fix these regressions. I haven't tested that PR much so I've probably broke new things while fixing part of the issues you reported. I plan to continue working on these slowly over the next few days.

[hannesm]

thanks, a fine test is mirage-skeleton. I'll comment in more detail when I manage to find time to test or review your PR.

[samoht]

So my PR is indeed broken -- i've spent a few hours trying to debug this, but the full_test.exe tests are really hard to get right...

I think we need to take a step back and try to define the semantics of -b and -f a bit more or we can indeed just remove these (which will make of that complexity disappear).

[Drup]

According to past experiences, attempts to remove -f (and -b) were not so beneficial, since we fundamentally need to understand all these interactions to write the frontend, regardless of the presence of these options.

If we decide to cleanup the frontend and remove all the fancy file-system related options, I think we still need to be careful about the design.

That being said, I was pretty sure those were exercised by @yallop 's tests ... did those get removed ?

[hannesm]

@samoht on 3rd of August:

Many thanks for the feedback, I'll address these issues next week and I'll add a few tests to avoid this kind of regressions in the future.

now, a month later, this is still not fixed and released? :/
should we in the meantime remove functoria 3.0 from opam-repository (or restrict mirage to functoria < 3.0.0) until the regressions are fixed? it's slightly cumbersome for (a) new developers and (b) me who always forgets to pin functoria to 2.2.4 to be able to build various unikernels.

[samoht]

This is fixed in #176 but it's very hard to test properly so I am not sure how to go forward. I'll revert the 3.0 release in the meantime.

[samoht]

See ocaml/opam-repository#14776

[avsm]

i'm normally reluctant to remove a release from opam-repo, but the regressions seem significant enought to warrant this, so done.

[samoht]

This is now fixed and a 3.0.1 release is on its way:ocaml/opam-repository#15099