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

Convert from jbuilder to dune#158

Merged
Drup merged 5 commits intomirage:masterfrom
emillon:dune
Oct 15, 2018
Merged

Convert from jbuilder to dune#158
Drup merged 5 commits intomirage:masterfrom
emillon:dune

Conversation

@emillon
Copy link
Copy Markdown
Contributor

@emillon emillon commented Oct 4, 2018

This uses dune-the-binary, dune-the-file-format as well as some idioms ((test) stanza, :standard). I'm not sure if it's worth passing -safe-string by hand these days - if that's not the case we can just remove (flags) completely.

@emillon emillon force-pushed the dune branch 2 times, most recently from 3db0409 to 2add16b Compare October 4, 2018 20:54
app/dune Outdated
(name functoria_app)
(public_name functoria.app)
(libraries functoria dynlink cmdliner rresult fmt.tty fmt.cli ocamlgraph astring fpath bos logs logs.cli logs.fmt)
(flags -bin-annot -safe-string -w "A-40-42-44-48" -warn-error "+1..49" -warn-error "-15")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know this has been source of disagreement from @Drup in the past; @Drup are you happy to switch back to use the default standard flags here?

@hannesm
Copy link
Copy Markdown
Member

hannesm commented Oct 5, 2018

@emillon I'm fine with using dune defaults, and not pass -safe-string manually in the mirage ecosystem

@emillon
Copy link
Copy Markdown
Contributor Author

emillon commented Oct 5, 2018

@hannesm thanks, I removed (flags) to get the default ones.

@Drup
Copy link
Copy Markdown
Member

Drup commented Oct 6, 2018

Can you remind me of which warning/errors are going to be enable/disable because of this? The warnings were carefully chosen.

@emillon
Copy link
Copy Markdown
Contributor Author

emillon commented Oct 8, 2018

The warnings numbers correspond to the following:

 40 Constructor or label name used out of scope.
 42 Disambiguated constructor or label name (compatibility warning).
 44 Open statement shadows an already defined identifier.
 48 Implicit elimination of optional arguments.
 15 Private method made public implicitly.

I'm not sure why -warn-error -15 is used since by default it is not a fatal warning.

@Drup
Copy link
Copy Markdown
Member

Drup commented Oct 15, 2018

No, I meant, what is the difference between our warnings, and the one enable/disabled by dune by default.

@emillon
Copy link
Copy Markdown
Contributor Author

emillon commented Oct 15, 2018

It seems that dune suppresses the additional following warnings:

4 Fragile pattern matching: matching that will remain complete even
if additional constructors are added to one of the variant types
matched.
29 Unescaped end-of-line in a string constant (non-portable code).
41 Ambiguous constructor or label name.
45 Open statement shadows an already defined label or constructor.
58 Missing cmx file
60 Unused module declaration

@Drup
Copy link
Copy Markdown
Member

Drup commented Oct 15, 2018

Right, the additional ones are fairly debatable, so I'm okay with using the defaults.

@emillon
Copy link
Copy Markdown
Contributor Author

emillon commented Oct 15, 2018

Thanks. I just rebased this on top of master.

@Drup Drup merged commit 242829e into mirage:master Oct 15, 2018
hannesm added a commit to hannesm/opam-repository that referenced this pull request Nov 16, 2018
CHANGES:

* compute all transitive opam dependencies for info (mirage/functoria#151, by @hannesm)
* support pin-depends in generated opam file (mirage/functoria#163, by @hannesm)
* use dune as build system (mirage/functoria#158, by @emillon)
* use Ptime for time printing (mirage/functoria#160, by @emillon)
* inject global arguments into generated header (mirage/functoria#159, by @emillon)
* add Functoria_key.add_to_context (mirage/functoria#161, by @emillon)
* output opam2 files (mirage/functoria#157, by @hannesm)
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.

4 participants