-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
There was a problem hiding this 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:
Line 252 in 545e7f5
(defn expand-shas! |
But that's something to tackle on a different PR
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
498bf3a
to
3f8a1ed
Compare
- Strip out empty lines from fake-git tag command
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.
I could also just comment out the tests. |
@mjmeintjes thanks, let's keep the tests, I'll try to find why are failing only on CI |
pkgs/fake_git.clj
Outdated
|
||
(defn -main [] | ||
(let [res (result (cli/parse-args *command-line-args*))] | ||
(doseq [line res] |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
test/integration/fake_git_test.clj
Outdated
(defn- run-fake-git [project-dir cmd] | ||
(sh/sh | ||
"bash" "-c" | ||
(str "nix run .#fake-git -- --git-dir " project-dir " " cmd))) |
There was a problem hiding this comment.
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 [...] }
There was a problem hiding this comment.
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.
Instead of as a script
It seems that the |
@mjmeintjes thanks, on my local babashka clone I run
I'm going to update it |
I updated the lock files, and updated @mjmeintjes I apologize for the delay in addressing this PR, but thanks again for your work! |
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: