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

Better gitlibs support #120

Merged
merged 9 commits into from
Jun 13, 2024
Merged

Conversation

mjmeintjes
Copy link
Contributor

This adds support for dependencies that specify :git/tag and shortened :git/sha

Also fixes #108 "Manifest not found error" for dependencies using :deps/root

Changes:

  • Pre-calculate information about rev ancestors and store in lock file
  • Bump lock file version
  • Store git repo info in nix store under ${git-dir}/revs/REV
  • Add fake git command to provide git tags and rev ancestor information at build time using the git repo info stored in ${git-dir}/revs/REV (fixes Consider adding a fake git command #61)

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 a lot for your PR! I took a quick look and added some comments. I want to take a second look with more calmness, but overall it looks good. :)

I like the fake git approach. It will probably allow us to remove this function:

(defn expand-shas!

But that's something to tackle on a different PR

src/cljnix/git.clj Outdated Show resolved Hide resolved
pkgs/fakeGit.nix Outdated Show resolved Hide resolved
pkgs/fake-git.py Outdated Show resolved Hide resolved
This adds support for dependencies that specify :git/tag and shortened
:git/sha

Also fixes "Manifest not found error" for dependencies using :deps/root

Changes:
- Pre-calculate information about rev ancestors and store in lock file
- Bump lock file version
- Store git repo info in nix store under ${git-dir}/revs/REV
- Add fake git command to provide git tags and rev ancestor information
  at build time using the git repo info stored in ${git-dir}/revs/REV
@mjmeintjes
Copy link
Contributor Author

Can you have a look at the tests in fake_git_test.clj to see why they are failing? They pass when I run them locally, and I don't know enough about Github Actions to figure out why they are failing.

❯ kaocha
       unit:   100% [==================================================] 13/13
integration:   100% [==================================================] 15/15
28 tests, 61 assertions, 0 failures.

I could also just comment out the tests.

@jlesquembre
Copy link
Owner

@mjmeintjes thanks, let's keep the tests, I'll try to find why are failing only on CI


(defn -main []
(let [res (result (cli/parse-args *command-line-args*))]
(doseq [line res]
Copy link
Owner

Choose a reason for hiding this comment

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

Why doseq? Looks like it's only used to print the vector of string in multiple lines

Can we update result to return
{:exit 0 :out []} ,
{:exit 1 :out []},
{:exit 0 :out ["v0.0.1" "v0.0.2"]}, ...

And then do a (->> res :out (string/join "\n") println)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made these changes as suggested.

Comment on lines 12 to 15
(defn- run-fake-git [project-dir cmd]
(sh/sh
"bash" "-c"
(str "nix run .#fake-git -- --git-dir " project-dir " " cmd)))
Copy link
Owner

Choose a reason for hiding this comment

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

I want to avoid calling external programs in clojure tests. For those cases we can use bats, but since we are invoking a bb script, why not calling that code directly from clojure?

Could you update the babashka script to have 2 main functions? e.g.: main and main*

main is the real entry point when invoked with babashka, it should be quite minimal and invoke main*
main* returns only data, and we call this function from the clojure tests.

In deps.edn, we can add pkgs in the :test {:extra-pahts [...] }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the fake-git script and tests as suggested, tests pass on my side.

To enable calling the fake-git script from clojure for testing.
@mjmeintjes
Copy link
Contributor Author

It seems that the extra-pkgs/babashka/deps-lock.json needs to be regenerated to work with the new lock file version. How do I do this?

@jlesquembre
Copy link
Owner

@mjmeintjes thanks, on my local babashka clone I run

nix run ~/projects/clj-nix#deps-lock -- --lein --deps-exclude resources/META-INF/babashka/deps.edn

I'm going to update it

@jlesquembre
Copy link
Owner

I updated the lock files, and updated calc-ancestors to return a sorted-map, I'm waiting for CI to finish before merging it.

@mjmeintjes I apologize for the delay in addressing this PR, but thanks again for your work!

@jlesquembre jlesquembre merged commit 4b9704b into jlesquembre:main Jun 13, 2024
1 check passed
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.

Manifest file not found error Consider adding a fake git command
2 participants