kit icon indicating copy to clipboard operation
kit copied to clipboard

Module sync issue - file module.edn modified after kit/sync-modules

Open markokocic opened this issue 2 years ago • 8 comments

Steps to reproduce:

  1. Create a new project, e.g deps -Tnew create :template io.github.kit-clj :name marko/testm
  2. start repl and sync modules (kit/sync-modules)
  3. go to modules/kit-modules directory
  4. git status shows modules.edn as modified file.
diff --git a/modules.edn b/modules.edn
index 324ac65..6953edb 100644
--- a/modules.edn
+++ b/modules.edn
@@ -1,20 +1 @@
-{:name "kit-modules"
- :modules
- {:kit/html
-  {:path "html"
-   :doc "adds support for HTML templating using Selmer"}
-  :kit/htmx
-  {:path "htmx"
-   :doc "adds support for HTMX using hiccup"}
-  :kit/metrics
-  {:path "metrics"
-   :doc "adds support for metrics using prometheus through iapetos"}
-  :kit/sql
-  {:path "sql"
-   :doc "adds support for SQL. Available profiles [ :postgres :sqlite ]. Default profile :sqlite"}
-  :kit/cljs
-  {:path "cljs"
-   :doc "adds support for cljs using shadow-cljs"}
-  :kit/nrepl
-  {:path "nrepl"
-   :doc "adds support for nREPL"}}}
+{:name "kit-modules", :modules {:kit/html {:path "html", :doc "adds support for HTML templating using Selmer"}, :kit/htmx {:path "htmx", :doc "adds support for HTMX using hiccup"}, :kit/metrics {:path "metrics", :doc "adds support for metrics using prometheus through iapetos"}, :kit/sql {:path "sql", :doc "adds support for SQL. Available profiles [ :postgres :sqlite ]. Default profile :sqlite"}, :kit/cljs {:path "cljs", :doc "adds support for cljs using shadow-cljs"}, :kit/nrepl {:path "nrepl", :doc "adds support for nREPL"}}, :module-root "kit-modules"}^M

Since the file is modified, further syncs fails. This happens on windows.

After reverting modules.edn file using git checkout -- . it is not possible to install module

  1. restart repl and do, e.g (kit/install-module :kit/cljs)
  2. the following error appears
user=> (kit/install-module :kit/cljs)
Execution error (FileNotFoundException) at java.io.FileInputStream/open0 (FileInputStream.java:-2).
modules\cljs\config.edn (The system cannot find the path specified)
user=>

In order to fix this, I have to manually move content of kit-modules directly under modules directory. After that I am able to install a module.

  1. cd modules
  2. mv kit-modules/* .
  3. start repl and do (kit/install-module :kit/cljs),
  4. it works

Note: it's not related to :kit/cljs module. Same behaviour for any module

Btw, installing a module right after doing kit/sync-modules (without reverting the modified modules.edn file) works properly.

Could be that something gets messed up with path.

markokocic avatar Jul 27 '22 16:07 markokocic

Btw, I just saw that kit is using clj-git for interaction with modules. Why not just use normal git from shell? I can't imagine a case a developer using kit will not have git installed.

Even further, why git at all? Why not point directly to any url in kit-config.edn, and fetch it from there, using regular http?

markokocic avatar Jul 28 '22 12:07 markokocic

Git makes it easier to sync changes when the repo is updated. I think going with HTTP could result in having to reimplement a lot of git features over time. And I generally prefer using native code instead of shelling out as it makes things more portable. I've fielded a lot of issues from windows users in the past for Luminus builds, and a lot of them stem from doing system interop.

yogthos avatar Jul 31 '22 15:07 yogthos

This is one example of windows issues ;)

I understand the rationale behind git, but when I look at the modules as a kind of repository, compared to maven, npm or any other repo, I don’t see a use case where someone would be interested in a repo history.

sync-modules doesn’t even need to fetch all modules locally, it can fetch modules.edn, which contains a list of modules with urls to fetch. If later we want versioning, it’s just adding /v2 to the base url, while managing multiple per module version tags may be more challenging.

Installing a module would be a fetch from the url and applying the rest as we do now.

markokocic avatar Jul 31 '22 16:07 markokocic

Looks like the modules.edn file is modified after running (kit/sync-modules) because kit.generator.modules/sync-modules! function is updating it and adds :module-root "kit-modules" to it. This doesn't gook right.

@yogthos , @nikolap Why do we need to add :module-root to modules.edn after sync? Could it be inferred from the directory name? Or from the module config in kit.edn, where we already have :modules :root and a :name for every repository?

markokocic avatar Aug 10 '22 11:08 markokocic

This pull request takes the modules root from the name of the folder containing modules.edn.

markokocic avatar Aug 10 '22 12:08 markokocic

I think the intent was to permit multiple repositories as sources for modules. @yogthos can you confirm?

nikolap avatar Aug 15 '22 09:08 nikolap

Ok. Pull Request should keep the same behavior as before, just without modification of modules.edn after sync.

I tested it on windows, if you can confirm it works on other platforms, it should be ready for merging.

markokocic avatar Aug 15 '22 10:08 markokocic

Yeah, right now we support registering multiple module repos, and we want to avoid collisions.

yogthos avatar Aug 15 '22 15:08 yogthos