Remove process/exec for git binaries

[jeroenvandijk]

Test with

bb bbin install io.github.borkdude/carve --as carve-dev && cat $(which carve-dev) && carve-dev ; time carve-dev

0.069

Vs

bbin install io.github.borkdude/carve --as carve-wrapped && cat $(which carve-wrapped) && carve-wrapped ; time carve-wrapped

0.100

Please answer the following questions and leave the below in as part of your PR.

• This PR corresponds to an issue with a clear problem statement.
  • No need to wrap git binary with process/exec #50

I need to fix the tests, but this is a first proof of concept that seems to work.

• Look into failing tests

[jeroenvandijk]

@rads I have two tests failing locally without my changes (with my changes a few more though). I don't see this on the main branch here. I'll investigate a bit later. Never mind, these are the private repo tests.

[borkdude]

Maybe we should prevent running the private repo tests for GITHUB_ACTOR != rads?

[rads]

@jeroenvandijk @borkdude: My intent was to set things up so we can still run the private repo tests without being tied to a specific environment. The private repos can be checked out using the git-wrapper instead of git, which overrides the Git SSH command to include the private key for the test repos. (If there are any alarms going off about storing a private key in a public repo, rest assured that this private key is only used for the test repos and only grants read access.)

That said, I think there may be an issue with how I swapped out the Git command using the clojure.gitlibs.command property. I'm only setting it right now in the test runner, not the subprocesses, so I'm confused why it's actually working in GitHub Actions on main (where there is no private key outside of the bundled one in the repo). I'm not sure if deps.clj supports the clojure.gitlibs.command property or GITLIBS_COMMAND env var since it's not actually using tools.gitlibs directly as far as I know.

[borkdude]

@rads GITLIBS_COMMAND should work (if that is supported by the clojure CLI)

[rads]

You can run this to check if the git-wrapper is working as intended locally:

./test-resources/git-wrapper clone [email protected]:radsmith/bbin-test-lib-private.git

[rads]

@borkdude: Good to know, I can try using the env var instead of the system property (ensuring that subprocesses also have it set) and see if that fixes it.

[jeroenvandijk]

@rads ./test-resources/git-wrapper clone [email protected]:radsmith/bbin-test-lib-private.git works for me.

I've tried to export the GITLIBS_COMMAND and run it with bb test, but no success here, maybe I'm doing it wrong.

[rads]

If the git-wrapper is working for you, that's good news. Now we just have to ensure that the git-wrapper is being used when calling run-bin-script in the tests. If that's the case and the tests are still failing then the test failures are likely due to separate issue.

[borkdude]

Btw, locally the tests are failing for me like this, but it seems the private git repo works (without changes in my env):

Testing babashka.bbin.cli-test

Testing babashka.bbin.scripts-test

FAIL in (install-tool-from-local-root-test) (/Users/borkdude/dev/bbin/test/babashka/bbin/scripts_test.clj:239)
install ./ --tool
expected: (str/includes? (run-bin-script "footool" "k" ":a" "1") "(:a)")
  actual: (not (str/includes? "" "(:a)"))

FAIL in (install-tool-from-local-root-test) (/Users/borkdude/dev/bbin/test/babashka/bbin/scripts_test.clj:239)
install ./ --tool
expected: (str/includes? (run-bin-script "footool" "v" ":a" "1") "(1)")
  actual: (not (str/includes? "" "(1)"))

[jeroenvandijk]

If we solve the Git wrapper thing, tests should pass now. The last two that are failing are failing like mentioned by @borkdude.

Having said that, the implementation I just added feels quite bad and it needs a separate test for the -m option I believe. When I remove this branch in the code. The same two tests fail, but due to the git wrapper I assume.

[rads]

@jeroenvandijk: Those failing tests are installing from the file system rather than Git so I’m thinking it’s something else.

[rads]

Regarding the code quality: I plan to refactor the installation functions as part of the next chunk of work, so I’m fine with the code being messy right now as long as the tests pass.

In the future, I will likely add a protocol for installing and set up each procurer/artifact pair as its own implementation, which will require rewriting most of the existing installation code. The tests should stay the same though.

[jeroenvandijk]

@jeroenvandijk: Those failing tests are installing from the file system rather than Git so I’m thinking it’s something else.

Ah true, I wasn't paying enough attention. So the Git tests are working then, sorry. Getting late here.

[jeroenvandijk]

@jeroenvandijk: Those failing tests are installing from the file system rather than Git so I’m thinking it’s something else.

@rads I've reverted the commits to see if the tests would be green again (locally they are not), but CI is now showing the same errors as locally, without any diff with main.

[rads]

@jeroenvandijk: Should we keep this PR open still?

[jeroenvandijk]

@rads No it can be closed. There is some work left and I need to dive into it again. I'll create a new PR when I get to it.