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 support for deps-new #52

Open
rads opened this issue Jul 23, 2022 · 11 comments
Open

Add support for deps-new #52

rads opened this issue Jul 23, 2022 · 11 comments
Labels
enhancement New feature or request

Comments

@rads
Copy link
Contributor

rads commented Jul 23, 2022

Overview

I'm working on a PR for neil (babashka/neil#51) which will make it possible to simplify the "Quick start" for Kit:

brew install clojure babashka/brew/neil
neil new io.github.kit-clj/kit yourname/app

Requirements

Performance

The neil new command is using babashka to run org.corfield.new/create which removes the JVM startup time. I haven't ported the lein-template to deps-new so I can't do a direct comparison yet. That said, with the built-in app template, the difference is significant (3s down to 0.1s):

$ time clojure -Tnew app :name yourname/app :force true
Generating a project called app based on the 'app' template.
clojure -Tnew app :name yourname/app :force true  3.04s user 0.13s system 205% cpu 1.542 total

$ time ./neil new app yourname/app --overwrite
Creating project from org.corfield.new/app in app
./neil new app yourname/app --overwrite  0.10s user 0.03s system 88% cpu 0.147 total
@yogthos
Copy link
Collaborator

yogthos commented Jul 23, 2022

This sound good to me, @nikolap thoughts?

@nikolap
Copy link
Member

nikolap commented Jul 24, 2022

Sounds great. Seems like a good option to migrate (simpler command, faster startup).

@rads
Copy link
Contributor Author

rads commented Jul 24, 2022

@nikolap @yogthos: I have an initial version of the port here: https://github.com/rads/kit/tree/master/libs/deps-template

Still needs some clean up, but it works! Here's how you can test it yourself:

  1. Ensure you have the latest version of neil:
    brew upgrade babashka/brew/neil
  2. Create a kit project with +xtdb and +quartz
    time neil new io.github.kit-clj/kit yourname/app --xtdb --quartz \
      --git/url https://github.com/rads/kit.git \
      --git/sha cb2236503c5fd5202df28bca1551bd2128a71e23
    
    Creating project from io.github.kit-clj/kit in app
    ... 0.10s user 0.04s system 95% cpu 0.147 total

Edit: Updated the instructions to only two steps now that neil new is available in the main package.

@rads
Copy link
Contributor Author

rads commented Jul 31, 2022

The neil new feature is now available in version 0.1.36: https://github.com/babashka/neil/blob/main/CHANGELOG.md#0136

Now that neil is ready to go, all we need to do is add the support for deps-new to kit-clj.

As seen above, I've already got a prototype of this working on my branch. I'm planning to open a PR soon so we can start reviewing the code and talking about how this fits in to the existing kit-clj project.

@rads
Copy link
Contributor Author

rads commented Aug 6, 2022

@nikolap @yogthos: I'm planning to start working on this again to hopefully get it across the finish line. Do you folks have any specific ideas of how this would fit in with the existing lein-template? Here is what I'm thinking so far:

  • Goal: This change is backwards compatible
    • The lein-template lib and clj-new templates will continue to be supported
    • The deps-template lib will be added to libs and referenced from the top-level deps.edn
    • To avoid duplication, a third base-template lib will be added to libs to share things between lein-template and deps-template

In my WIP PR, I didn't really have to change the template files themselves to make things work with deps-new. It's mainly the build tooling and manifest files that are different. Therefore I think it would be reasonable to share the template files across both lein-template and deps-template. Once I have a plan for how to reconcile with lein-template, I can clean up the actual build code in the PR since I haven't done that yet.

What do you think about this?

@yogthos
Copy link
Collaborator

yogthos commented Aug 6, 2022

Yeah that sounds pretty reasonable to me.

@rads
Copy link
Contributor Author

rads commented Aug 9, 2022

@yogthos: It looks like deps-new currently only supports sourcing files from a single root template directory. If we try to break apart a single template into base-template and deps-template, the deps-template will not find the files in the resources of base-template, even if the directory names are correct on both sides (resources/io/github/kit_clj/kit).

In other words, it doesn't seem like it's possible to compose templates with deps-new right now. I think the easiest path forward would be to make lein-template depend on deps-template, which seems to work fine since clj-new doesn't have the same limitation.

Now that I've eliminated the duplication between the templates, I'm still working out bugs in the deps-template version. I'm planning to add a test that will check that the output of both lein-template and deps-template is the same.

@yogthos
Copy link
Collaborator

yogthos commented Aug 10, 2022

Yeah that sounds like the way to go since deps-new only supports using a single root.

@nikolap
Copy link
Member

nikolap commented Aug 15, 2022

Just saw in your issue on deps-new that it's possible to support multiple files, but agree with Dmitri that it's cleaner to just have lein-template depend on deps-template for backwards compatibility. That way we don't duplicate the code

@rads rads mentioned this issue Aug 18, 2022
@rads
Copy link
Contributor Author

rads commented Aug 18, 2022

@nikolap @yogthos: The first PR is ready for review: #65

This is just to set things up with the shared templates. Then the next PR will be to add the code to make deps-new work.

@markokocic markokocic added the enhancement New feature or request label Feb 28, 2023
@martinklepsch
Copy link
Contributor

Hey @rads, this seems like a great idea still, do you have a sense of what is still missing to get this working? Reading the comments here it looks like it was pretty close :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants