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

Add `dune init` command #1448

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
7 participants
@shonfeder
Copy link
Contributor

commented Oct 14, 2018

Towards completion of #159

This implements creation of new projects, as discussed in #159 (comment) and using the more simplistic, hard-coded strings approach encouraged here #159 (comment)

I think tests ensuring initialized projects can be build should be included in this PR, but I'd like to get the general shape of the implementation OKed before writing the test cases.

@shonfeder shonfeder requested review from diml, emillon and rgrinberg as code owners Oct 14, 2018

@shonfeder shonfeder force-pushed the shonfeder:init-command branch from f811db7 to 66304ff Oct 14, 2018

Show resolved Hide resolved src/init.ml Outdated
@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2018

(Looks like I need to make several small changes for older OCaml versions. I'm looking in to that now.)

@shonfeder shonfeder force-pushed the shonfeder:init-command branch from 66304ff to 2555b7a Oct 14, 2018

@rgrinberg
Copy link
Member

left a comment

I don't quite understand the necessity of functor or first class modules here. It seems like plain old records would work just as well here.

Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2018

I opted for modules to avoid having to thread the name and path parameters through a bunch of functions. However, I can definitely understand that the end result seems quite heavy duty given the simplicity of the feature added! Moreover, if we're not generating any content for the files, there's less need to thread those parameters through. I'll re-structure for a more lightweight approach.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

I opted for modules to avoid having to thread the name and path parameters through a bunch of functions. However, I can definitely understand that the end result seems quite heavy duty given the simplicity of the feature added! Moreover, if we're not generating any content for the files, there's less need to thread those parameters through. I'll re-structure for a more lightweight approach.

Thanks, we have situations in dune where we have a little too much boilerplate and we go with functors but the situation here is nowhere near as acute. The only case where we use first class modules are plugins, so introducing it here to get rid of a boilerplate is certainly unprecedented. I think the code will look quite clean with records by the way so boilerplate won't even be an issue.

@shonfeder shonfeder force-pushed the shonfeder:init-command branch from 2555b7a to 997eaeb Oct 15, 2018

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Notes on a few of the changes:

  • Replaced the baroque first class modules with records (I think I'm overeager for 1ML 😋).
  • Replaced the check for a preexisting project directory with checks for preexisting files during their creation. Now, if, e.g., a file exists at <project>/bin/main.ml, it will report this fact and skip creation. This protects against overwriting files (as the check for the root directory was meant to), but also allows initializing existing projects by fleshing out missing dune files that may be needed.
  • Removed the <project>/src/lib.ml file, but sneakily kept a "Hello, World" line in <project>/bin/main.ml. I think this is innocuous: It doesn't introduce the same complications as the content in <project>/src/lib.ml but it still provides the benefit of being able to get a printing program with one command. However, I'll remove this line without complaint if @rgrinberg is opposed.
Show resolved Hide resolved src/init.ml Outdated
@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Okay, looks really nice. Good work @shonfeder. Before I merge I'm planning the following UI changes:

  • Change $ dune init foo --lib to $ dune init lib foo. The reason for this is that we'd like $ dune init lib --help to only show the relevant options for lib.

  • For the executable template, we don't want an associated library. I think a single dune + source file is enough.

  • Let's remove the generation of all .opam files. First of all, it will be done properly from dune files later and second of all it's not helpful for non public libs and binaries anyway. And the new @all alias makes building private libraries and executables much easier so there's even less incentive for this to be the default.

  • Add the ability to put paths in the argument $ dune init <lib|exe> foo/bar should create a <lib|exe> named bar inside foo. There's no need directory nesting past this I believe.

If you see any problems or propose any changes to these suggestions, please feel free to discuss.

If you'd like some help in implementing some of this stuff, I'll gladly help out as well as I realize that some of these change aren't small.

We'll also need to add a test suite for this feature. I suggest that we add this test suite before we start any of the changes I've proposed. If you need help here, I'm happy to point you in the right direction or put up a few tests.

@emillon
Copy link
Collaborator

left a comment

Thanks! This nex feature will probably open up a healthy amount of bikeshedding but this is probably necessary!

Show resolved Hide resolved bin/main.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved src/init.ml Outdated
Show resolved Hide resolved bin/main.ml Outdated
Show resolved Hide resolved bin/main.ml Outdated
@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

By the way, this issue is relevant to make the opam files obsolete: #1112

There are no immediate plans to implement this (we are looking for volunteers), but it shouldn't be too hard and hopefully will be present in the next version or two

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

I have a question on one point, re: "Add the ability to put paths in the argument $ dune init <lib|exe> foo/bar". The current interface allows for dune init --lib my_project path/to/some/location. It seems to me preferable to keep this kind of behavior, rather than creating an idiosyncratic meaning for / and overloading the meaning of the first positional argument. So, it seems preferable to me to keep the current behavior.

Other than that, I don't have any problems with the proposed changes and I'm happy to make them. I will let know if I get stuck or bogged down, but I think I should be able to bring those changes about in the next several days.

Thanks for the guidance, corrections, and reviews so far, @rgrinberg and @emillon!

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

have a question on one point, re: "Add the ability to put paths in the argument $ dune init <lib|exe> foo/bar". The current interface allows for dune init --lib my_project path/to/some/location. It seems to me preferable to keep this kind of behavior, rather than creating an idiosyncratic meaning for / and overloading the meaning of the first positional argument. So, it seems preferable to me to keep the current behavior.

Well the current behavior isn't ideal as it's forcing a particular directory structure. Dune doesn't really recommend any directory structure and many existing projects and users will have their own preference regarding this. Therefore it seems best to make the feature usable with any kind of directory structure.

My suggestion was just one idea on how to add the flexibility to create w/e directory structure the user wants. I'm happy to hear other suggestions. The use cases I have in mind are:

  • How do I create a library in the current directory?
  • How do I create a binary in an existing directory?

Other than that, I don't have any problems with the proposed changes and I'm happy to make them. I will let know if I get stuck or bogged down, but I think I should be able to bring those changes about in the next several days.

Thanks, we appreciate your work.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Ah, I think I understand what you are after now: we're not talking about the root directory of the project but the directory in which the dune file declaring the library is created. I can see the use.

@rgrinberg, according to the plan sketched out at #159 (comment), the two features you mention

  • How do I create a library in the current directory?
  • How do I create a binary in an existing directory?

would be supplied by the dune add command. The way discussed there (and what I understand many people to have been requesting), dune init should prime more holistic project development, and provide a quick and easy jumping off place. Thus, I think it makes sense to have a second command dedicated to small utility operations like creating a dune file of a certain sort sort in such and such a directory.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

It seems like $ dune init and $ dune add have a very overlapping feature set however. In fact, if $ dune init was idempotent, I can't think of any situation where I'd want to use $ dune add.

Also, I'm not very sure how $ dune init maps to a single project by the way. Most of the projects that I deal define more more than just 1 executables and/or a library. Having $ dune init be used once and $ dune add n times makes the latter the far more useful command to implement.

Here's another suggestion, if we want $ dune init to initialize a whole project skeleton for us, then we can have a $ dune init project subcommand. I agree that such a command should indeed create a directory.

By the way, I think it would help to have a type like:

type common =
  { name : string
  ; in_ : Path.t
  }
type public = bool
type inline_tests
type init = (* pick your own name here *)
  | Executable of common * public * libraries
  | Library of common * public * libraries * inline_tests
  | Test of common * Lib_name.t (* the library that we are testing *)
  | Project of common (* This constructor would include Library + Test for example *)

The above makes it very clear what arguments anything that we can init can accept. It's just a sketch though, I wouldn't necessarily use tuples for example.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

I am OK with ditching the add command and just putting that functionality in init.

Rather than dune init project lib ... (pretty unwieldy) I suggest we follow the patterns of cargo and stack and use init for stuff that initializes things an existing project and use new for creating new projects. This will make it more concise, more accessible to new users, and more likely to be familiar to people coming from neighboring communities. (but this is all matter for another PR any how :) ).

I will build out the API for init along the lines you suggest: that makes good sense to me.

I'm hoping to have all the suggested changes in place and ready for review by the end of next week.

[edited to amend estimate on turnaround time for completion]

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

Rather than dune init project lib ... (pretty unwieldy)

Sorry, my suggestion $ due init {project,lib,exe}. I did not suggest any kind of additional nesting. What I meant was that all the extra directories would only be created in the project init type.

I suppose the main difference between between our tool and cargo/stack is the once-only restriction of the templates that I'm not so fond of. It's true that a tool that quickly generates dune boilerplate vs a tool that starts a new project aren't the same thing. But it seems like at least a flexible $ dune init will handle the new with enough templates for it.

Btw, I have no experience with cargo but $ cargo init doesn't look very good. I've used stack a lot more and have found their custom templates quite useful, but their non composability is quite annoying. It's quite ugly to have hspec, servant, and servant-hspec templates for example. We can try and do better in this respect.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

I thought I recalled this being an issue.... according to dbuenzli/cmdliner#24, cmdliner doesn't support nested subcommands. Does anyone know of a reasonable way around this?

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

Ah, in dbuenzli/cmdliner#95 there is mention of implementing sub commands manually using positional arguments. Before I dig in to that, I'd like to confirm that the increased complexity of manual implementation of subcommands here is worth the cost. We should perhaps also consider that, since dune init is no longer meant to create projects, it seems reasonable that someone might dune init --lib --bin --test some_name . to create test, executable, and library dune files with one command.... (or is the use of the single shared some_name for all a blocker?).

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

Just had the (obvious) realization that dune init {lib,test,exe} PROJECT_NAME [OPTIONS...] can be implemented as positional arguments, and there's no need for a nested sub-command. Please ignore the noise.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

If we intentionally choose a suboptimal CLI for dune just because it's more convenient we are going to regret it down the line.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

Just had the (obvious) realization that dune init {lib,test,exe} PROJECT_NAME [OPTIONS...] can be implemented as positional arguments, and there's no need for a nested sub-command. Please ignore the noise.

Yes, this is alright for now. Although note that it's still not optimal because the help shown will not be specialized by the type.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2018

Ah, ok. So we do want this to be a nested subcommand -- for the focused help message (and added flexibility, etc.). So, just to be sure we're on the same page and that I understand correctly what's wanted here: completion of this PR (in it's ideal form) is now dependent on implementing a custom extension to cmdliner's CLI parsing that supports nested subcommands with their own help menus. Correct?

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

Yup, that would be ideal. But the workaround you proposed is backwards compatible so I'm personally ok with delaying the fully featured CLI.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@shonfeder have a look at my last commit. I believe this root argument isn't necessary.

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@shonfeder I'm curious, why didn't you just use dune's own parsing code for parsing the stanzas? This way, I think we have some duplication in the code and it isn't very desirable.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@rgrinberg Can you please specify which part of the code you think introduces parsing duplication and what existing code you think I might use instead? Afaik, I do very little in the way of parsing here. The only exception that comes to mind is the temporary function https://github.com/ocaml/dune/pull/1448/files#diff-ec22fb5fe4ab585936d484f498556279R69, which has the very specific purpose of identifying conflicting stanzas from the already loaded Cst.t.

Edit: To clarify, I'm all for reducing code duplication if possible.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Also, note that we need to preserve comments, since we read in and then re-emit dune code in dune files that may be hand written.

Show resolved Hide resolved src/dune_init.ml Outdated

@shonfeder shonfeder force-pushed the shonfeder:init-command branch from 9b27533 to b458a2f Mar 12, 2019

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@shonfeder yeah, that's indeed the function I'm talking about. Why is there an issue here regarding comments btw? We are simply using the Cst.t to create Signature.t values.

My idea would be to get rid of Signature.t altogether and just use Dune_file.Stanzas.parse along with the Stanza.t type to detect exes/libs. Do you think that's reasonable?

Also note that we if we initialize a "super context", then we'll have all the parsed stanzas for free.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

My idea would be to get rid of Signature.t altogether and just use Dune_file.Stanzas.parse along with the Stanza.t type to detect exes/libs. Do you think that's reasonable?

Yep. That's a better idea for identifying conflicts. I'll make the change this weekend.

Why is there an issue here regarding comments btw?

The issue with the comments in general is this: since we aim to be able to extend existing dune files and stanzas, we have to preserve the comments which users may have added. This means that in general, we can't just work with the Stanza.t type, or else we'd lose the comments when we write the stanzas back to the file. We can use Stanza.t for conflict detection, but we cannot simply use Stanza.t once we move to inserting dependencies, because we'll end up erasing user comments.

Also note that we if we initialize a "super context", then we'll have all the parsed stanzas for free.

I wasn't aware of super_context. That's quite cool. I think this would face the same problem of wiping out user comments tho, no?

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

I wasn't aware of super_context. That's quite cool. I think this would face the same problem of wiping out user comments tho, no?

Indeed, but it it can be used for just identify conflicts. The dune file can then be re-parsed to cst's when necessary.

@rgrinberg rgrinberg force-pushed the shonfeder:init-command branch from 5bb10df to f01a5d0 Mar 19, 2019

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@rgrinberg In order for the planned code reuse to work in a sensible way, the new_stanza : Cst.t that we want to add to the file must also be converted to a Stanza.t. So far, I have not been able to find any existing function Cst.t -> Stanza.t (nor any function to provide a helpful segment of that transformation, such as Ast.t -> Stanza.t). Do you happen to know of a sensible way to make this conversation? (And if there is such a function, or straightforward composition, to be had, then I needn't bother with all the incidental complexity of super contexts or parsing in a new file, since I can just convert the already loaded stanzas : Cst.t list to a Stanza.t list directly.)

I would, of course, be happy to work out this plumbing myself, but if that is required, I think we should just mark this with a TODO and address it in a follow up PR. This PR has already suffered from enough gradual sprawl, feature creep, and friction from staleness, and it would really help with my momentum to land it without too much more ado.

@shonfeder shonfeder force-pushed the shonfeder:init-command branch from f01a5d0 to 8011aff Mar 22, 2019

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Yeah, that's fine with me as well. Let's do the following:

  • Makes sure everything is signed-off/rebased on master
  • Update the manual and add a little note that $ dune init is still in development and is subject to change without warning
  • Merge this PR.
@kevinji
Copy link
Contributor

left a comment

Found a few typos.

Show resolved Hide resolved test/blackbox-tests/test-cases/dune-init/run.t Outdated
Show resolved Hide resolved test/blackbox-tests/test-cases/dune-init/run.t Outdated

@shonfeder shonfeder force-pushed the shonfeder:init-command branch from 8011aff to 972b481 Mar 24, 2019

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Thanks for catching those, @kevinji -- fixed now.

@rgrinberg action items addressed. Unless something else comes up, I think this should be ready for merge once CI is finished.

@shonfeder

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

After discussion on #1972, I've learned I was misunderstanding how Dune_file.Stanzas.parse worked. Witb my understanding corrected, I think I can now eliminate the ad-hoc parsing this PR.

I also need to correct a documentation comment is added to that function in this PR.

@rgrinberg rgrinberg force-pushed the shonfeder:init-command branch from af48c10 to 54fb8c5 Mar 27, 2019

Add `dune init` command
Signed-off-by: Shon Feder <shon.feder@gmail.com>
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>

@rgrinberg rgrinberg force-pushed the shonfeder:init-command branch from 54fb8c5 to 5daa555 Mar 27, 2019

@rgrinberg

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

@shonfeder Please make this improvement in a subsequent PR. Thank you for all the hard work on this. Let's track future improvements in other tickets.

@rgrinberg rgrinberg merged commit b137621 into ocaml:master Mar 27, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
DCO All commits are signed off!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@shonfeder shonfeder deleted the shonfeder:init-command branch Mar 27, 2019

rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Apr 9, 2019

[new release] dune (1.9.0)
CHANGES:

- Warn when generated `.merlin` does not reflect the preprocessing
  specification. This occurs when multiple stanzas in the same directory use
  different preprocessing specifications. This warning can now be disabled with
  `allow_approx_merlin` (ocaml/dune#1947, fix ocaml/dune#1946, @rgrinberg)

- Watch mode: display "Success" in green and "Had errors" in red (ocaml/dune#1956,
  @emillon)

- Fix glob dependencies on installed directories (ocaml/dune#1965, @rgrinberg)

- Add support for library variants and default implementations. (ocaml/dune#1900,
  @TheLortex)

- Add experimental `$ dune init` command. This command is used to create or
  update project boilerplate. (ocaml/dune#1448, fixes ocaml/dune#159, @shonfeder)

- Experimental Coq support (fix ocaml/dune#1466, @ejgallego)

- Install .cmi files of private modules in a `.private` directory (ocaml/dune#1983, fix
  ocaml/dune#1973 @rgrinberg)

- Fix `dune subst` attempting to substitute on directories. (ocaml/dune#2000, fix ocaml/dune#1997,
  @rgrinberg)

- Do not list private modules in the generated index. (ocaml/dune#2009, fix ocaml/dune#2008,
  @rgrinberg)

- Warn instead of failing if an opam file fails to parse. This opam file can
  still be used to define scope. (ocaml/dune#2023, @rgrinberg)

- Do not crash if unable to read a directory when traversing to find root
  (ocaml/dune#2024, @rgrinberg)

- Do not exit dune if some source directories are unreadable. Instead, warn the
  user that such directories need to be ignored (ocaml/dune#2004, fix ocaml/dune#310, @rgrinberg)

- Fix nested `(binaries ..)` fields in the `env` stanza. Previously, parent
  `binaries` fields would be ignored, but instead they should be combined.
  (ocaml/dune#2029, @rgrinberg)

- Allow "." in `c_names` and `cxx_names` (ocaml/dune#2036, fix ocaml/dune#2033, @rgrinberg)

rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Apr 9, 2019

[new release] dune (1.9.0)
CHANGES:

- Warn when generated `.merlin` does not reflect the preprocessing
  specification. This occurs when multiple stanzas in the same directory use
  different preprocessing specifications. This warning can now be disabled with
  `allow_approx_merlin` (ocaml/dune#1947, fix ocaml/dune#1946, @rgrinberg)

- Watch mode: display "Success" in green and "Had errors" in red (ocaml/dune#1956,
  @emillon)

- Fix glob dependencies on installed directories (ocaml/dune#1965, @rgrinberg)

- Add support for library variants and default implementations. (ocaml/dune#1900,
  @TheLortex)

- Add experimental `$ dune init` command. This command is used to create or
  update project boilerplate. (ocaml/dune#1448, fixes ocaml/dune#159, @shonfeder)

- Experimental Coq support (fix ocaml/dune#1466, @ejgallego)

- Install .cmi files of private modules in a `.private` directory (ocaml/dune#1983, fix
  ocaml/dune#1973 @rgrinberg)

- Fix `dune subst` attempting to substitute on directories. (ocaml/dune#2000, fix ocaml/dune#1997,
  @rgrinberg)

- Do not list private modules in the generated index. (ocaml/dune#2009, fix ocaml/dune#2008,
  @rgrinberg)

- Warn instead of failing if an opam file fails to parse. This opam file can
  still be used to define scope. (ocaml/dune#2023, @rgrinberg)

- Do not crash if unable to read a directory when traversing to find root
  (ocaml/dune#2024, @rgrinberg)

- Do not exit dune if some source directories are unreadable. Instead, warn the
  user that such directories need to be ignored (ocaml/dune#2004, fix ocaml/dune#310, @rgrinberg)

- Fix nested `(binaries ..)` fields in the `env` stanza. Previously, parent
  `binaries` fields would be ignored, but instead they should be combined.
  (ocaml/dune#2029, @rgrinberg)

- Allow "." in `c_names` and `cxx_names` (ocaml/dune#2036, fix ocaml/dune#2033, @rgrinberg)

- Format rules: if a dune file uses OCaml syntax, do not format it.
  (ocaml/dune#2014, fix ocaml/dune#2012, @emillon)
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.