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

Improve/fix Leiningen usage #145

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Conversation

JohnnyJayJay
Copy link
Contributor

This PR changes/fixes 3 small things that improve the usage of clj-nix for Leiningen users:

  • Fixing the faulty assertions for the withLeiningen module option (closes Unsatisfiable condition for option withLeiningen #144 )
  • Propagating withLeiningen to the derivation as enableLeiningen (so the option actually does something)
  • Adding an option --lein-profiles to the deps-lock CLI that allows one to set the profile(s) to use when generating the lock file. This is useful to have because currently, all custom profiles are merged (which can result in unwanted configuration outcomes that break the build, e.g. due to :repositories or :offline? options).

Copy link
Owner

@jlesquembre jlesquembre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Look good.
CI tests are failing, but that's related to the latest graalvm update on nixpkgs, nothing to do this your PR.
I want to run a couple of tests before merging

@@ -73,6 +73,7 @@ in
inherit (cfg) projectSrc name version main-ns buildCommand
lockfile java-opts compileCljOpts javacOpts
builder-extra-inputs builder-java-opts builder-preBuild builder-postBuild;
enableLeiningen = cfg.withLeiningen;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 that was the idea, thanks!

@JohnnyJayJay
Copy link
Contributor Author

JohnnyJayJay commented Oct 10, 2024

I want to run a couple of tests before merging

Yeah this currently isn't tested well. I'm also throwing it at my use case right now.

Edit: The --lein-profiles option doesn't seem to be working yet. Not sure why that is though

@jlesquembre
Copy link
Owner

@JohnnyJayJay I'm not familiar with Leiningen, I didn't test the --lein-profiles, but for my primary use case (building babashka) works as expected.
Let me know if you want to investigate the issue further. Otherwise, I suggest removing the --lein-profiles option and merge the other commits.

@JohnnyJayJay
Copy link
Contributor Author

@JohnnyJayJay I'm not familiar with Leiningen, I didn't test the --lein-profiles, but for my primary use case (building babashka) works as expected. Let me know if you want to investigate the issue further. Otherwise, I suggest removing the --lein-profiles option and merge the other commits.

I definitely want to investigate further. Something very strange is happening with the with-profiles thing in general that breaks my builds.

@JohnnyJayJay
Copy link
Contributor Author

Aha, I found the issue – the :user profile configuration generated by clj-nix is ignored if with-profiles is used, leading to missing dependencies. Fixing it by always adding the user profile.

@JohnnyJayJay
Copy link
Contributor Author

It's good to be merged from my side now.

@jlesquembre
Copy link
Owner

Thanks!

@jlesquembre jlesquembre merged commit 025a133 into jlesquembre:main Oct 11, 2024
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.

Unsatisfiable condition for option withLeiningen
2 participants