Slim/genclass - Githubissues

slimslenderslacks commented 1 year ago

@jlesquembre this isn't ready to merge yet but I was just looking at how we might verify that the main-ns has a :gen-class in mkCljBin. I was having a hard time getting the Latest SNAPSHOT version is used in mvn-deps-test to pass. Should I be updating that expected version for that test since the SNAPSHOT has moved?

I'm also seeing that one of the integration tests is hanging for me but I wouldn't have thought it was related to my change.

So far, I'm only warning about missing :gen-class for deps.edn projects but I'm not actually failing the build yet. I still need to figure out whether I can run the same check for both deps.edn and leiningen projects.

jlesquembre commented 1 year ago

thanks for working on it! Some notes:

Adding new dependencies to deps.edn is tricky. After adding (or updating) a dependency, you have to update builder-lock.json. It's bootstrapping issue, I should write some documentation about it. But in short, those dependencies are used by clj-nix when you build your clojure project. We read builder-lock.json and generate a CLASSPATH with nix. To simplify the process, the deps.edn file in clj-nix has some restrictions. In this PR you are adding a new dependecy, rewrite-clj, which is a transitive dependency of rewrite-edn (which we already have). rewrite-edn currently depends on rewrite-clj 1.1.45. At nix build time, clj-nix will use that version (we generate the CLASSPATH with nix here: https://github.com/jlesquembre/clj-nix/blob/main/pkgs/cljBuilder.nix) Not sure if that is a problem, but something to keep in mind. I'd prefer to remove rewrite-clj from deps.edn and use the same version used by rewrite-edn
The SNAPSHOT tests are fragile, if a new SNAPSHOT version is released, the test fails and we have to update it manually. I tried to pick old version for those tests, expecting no new SNAPSHOT releases. Also, I think that the real issue here is that you are on a darwin architecture, I suspect that maven is doing something different depending on the architecture.
About leiningen projects, it's ok to add a check only for deps.edn projects, we can add it later if needed. But the only reason I added leiningen support was to build babashka, I don't think many clj-nix users use leiningen

slimslenderslacks commented 1 year ago

Okay, now that I know that aarch64-darwin might be the issue, I'll look into it more deeply. I think you're probably right about that too. I've been seeing some odd things with inconsistent nar hashing so I'll drill into a little bit more deeply and let you know what I find.

slimslenderslacks commented 1 year ago

@jlesquembre it turned out that the unit tests were probably failing on darwin because of the artifact->pom function. I made a change to sort the glob results so that any SNAPSHOT poms would be first in the list (there should be only one SNAPSHOT pom file in any directory in an .m2/repository so that should work).

I've made the :gen-class test mandatory since it sounds like we don't need to support mkCljBin running on projects that don't have a deps.edn at the root. And main-ns is really a mandatory parameter for mkCljBin, right? I'm treating it like it's mandatory but maybe we should add a more explicit error message if main-ns is nil.

jlesquembre commented 1 year ago

@slimslenderslacks Thanks, and sorry for the late reply. It makes sense to make the :gen-class test mandatory. main-ns is mandatory, I agree with you in that we should add a better error message if is null. I'm testing your changes, it looks good :)

One question I have is why you added pkgs.diffutils.

Before merging it, could you take a look to my comments? The only thing I'd like to change before merging are the formatting changes. BTW, do you use a clojure formatter? I'm thinking about adding one to the project, but I'm not sure about it.

And is fine if you are not interested on doing those changes, I can do that on top of your PR.

slimslenderslacks commented 1 year ago

@jlesquembre I added pkgs.diffutils to the devshell because there was a cmp binary used somewhere ... Ya, it was https://github.com/jlesquembre/clj-nix/blob/main/test/new_project.bats#L38 and I didn't have it on my system so I thought it was probably appropriate to have this in the devshell. It was unrelated to my change I guess but I was trying to run those tests.

jlesquembre commented 1 year ago

I added pkgs.diffutils to the devshell because there was a cmp binary used somewhere ... Ya, it was https://github.com/jlesquembre/clj-nix/blob/main/test/new_project.bats#L38 and I didn't have it on my system

Good catch, I missed it, thanks :)

It looks good, I have some time planned to work on clj-nix this weekend, and merge this PR.

jlesquembre commented 1 year ago

@slimslenderslacks Thanks again, merged :)

jlesquembre / clj-nix

Slim/genclass #39