Skip to content

Conversation

@strub
Copy link

@strub strub commented Dec 10, 2020

See issue #40.

@rgrinberg
Copy link
Owner

Unfortunately, the yaml library contains C which makes it less portable than we'd like. What about introducing a separate package for the binary that we provide? This binary can definitely introduce the yaml dependency.

@strub
Copy link
Author

strub commented Dec 10, 2020

Or we make yaml an optional dependency?

@rgrinberg
Copy link
Owner

Optional dependencies are quite problematic and should be avoided as much as possible. We don't want our package to recompile just because the user might have chose to install the yaml packages. This might cause an unexpected build failure that might break the user's switch.

@strub
Copy link
Author

strub commented Dec 10, 2020

Ok, fine. I just want YAML in the CLI, whatever solution you prefer. Do you have in mind an opam package that does that? It is not crystal clear to me how you tell dune to take the installed library and not to (re)build it.

@rgrinberg
Copy link
Owner

Do you have in mind an opam package that does that? It is not crystal clear to me how you tell dune to take the installed library and not to (re)build it.

I was thinking the following:

  • Introduce a new package in this repository called mustache-cli (dune has no problem with multiple packages per repo). This package would depend on mustache, yaml, and whatever else is needed for the cli
  • Make the mustache binary a part of this new package.
  • The old mustache package will now contain only the library.

@gasche
Copy link
Collaborator

gasche commented Dec 25, 2020

I think that the last part of the PR ("change sections behaviour") is trying to fix #37, just like #41 and my now-merged #49. This part should not be necessary anymore.

(It's impressive how many different people hit this problem and tried to fix it in different ways! I suspect that your fix may have worked, and it's less invasive than #49, but it's probably better to get rid of the list concatenation on context entry anyway.)

@gasche
Copy link
Collaborator

gasche commented Dec 25, 2020

@rgrinberg I support your suggestion to split the binary in a different package with other dependencies, especially if you want the core library to remain Mirage-friendly. As the master of ceremony, I would encourage you to do the split (when you have time) :-)

@strub
Copy link
Author

strub commented Dec 25, 2020

Ok, I'll remove that commit. (And yes, I took the shortest path to something working :) )

@rgrinberg
Copy link
Owner

@strub could you rebase this PR please? I'll do the package split myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants