search

crystal-lang / shards

Dependency manager for the Crystal language
Other
763 stars 100 forks source link

Add fossil resolver #530

Closed MistressRemilia closed 2 years ago

MistressRemilia commented 2 years ago

This PR implements a resolver for Fossil repositories. The implementation is based mostly on the Git resolver, with some parts based on the Mercurial resolver, then tweaked to match the behavior of Fossil. The tests are based on the Mercurial tests since Fossil supports named branches.

I'm somewhat unsure of my changes to the .github/workflows/ci.yaml and .circleci/config.yaml since I've never used these features before, but I believe they're correct.

FWIW, make test ran to completion on my local machine, and I also did a quick test with a Fossil repo I already had set up.

This is to close #529

chillfox commented 2 years ago

Is there any chance this will be included in the next shards version?

chillfox commented 2 years ago

I had a look at the circleci tests and they are failing because the USER environment variable is not set. There is a list of builtin variables here https://circleci.com/docs/2.0/env-vars/#built-in-environment-variables Of those CIRCLE_USERNAME may not always be available, so maybe use CIRCLE_PROJECT_USERNAME, or you could try to see if the whoami or who command is available in the environment and set it using one of those.

MistressRemilia commented 2 years ago

Thank you! I've implemented that change and we'll see if it works. CI stuff is still very foreign to me.

straight-shoota commented 2 years ago

The code quality is really great, thank you!

In particular, the workarounds for older fossil versions are great. However, I'm wondering a bit whether they are actually necessary. Workarounds add complexity to our code. Since shards doesn't have any support for fossil, we don't have to care for backwards compatibility. When we add it, we can just state the minimum compatible version is something like 2.14 (which was released a year ago).

I don't have any fossil usage experience. So I really can't tell if it would be too restrictive. But considering some of the more recent enhancements seem to be pretty useful, I would expect fossil users to probably be tracking recent releases more closely?

chillfox commented 2 years ago

I don't have any fossil usage experience. So I really can't tell if it would be too restrictive. But considering some of the more recent enhancements seem to be pretty useful, I would expect fossil users to probably be tracking recent releases more closely?

The version in various package repositories can be quite old, but considering how easy it is to install by just downloading and placing the binary in /usr/local/bin/ I don't think it would be unreasonable to require a newer version.

MistressRemilia commented 2 years ago

However, I'm wondering a bit whether they are actually necessary. Workarounds add complexity to our code.

Since shards doesn't have any support for fossil, we don't have to care for backwards compatibility. When we add it, we can just state the minimum compatible version is something like 2.14 (which was released a year ago).

I kind of expect the same thing, but I also can see situations where an end user who's also a power user who just wants to do a sudo apt-get install fossil then run Shards. Also the image that the test-on-nightly job was using seemed to have an ancient version (2.10).

The workarounds I put in were only the bare minimums to support feature parity with Git and Mercurial with some of the more obscure (to me, anyway) ways that Git and Mercurial are used by Shards behind the scenes. But if you feel they should be removed, I'd be more than happy to remove them ^_^

straight-shoota commented 2 years ago

I suppose it's probably best then to leave those workarounds in for now. You already invested to implement them, so it seems bad to just throw it out and exclude users of older versions. We can always do that once maintainting the workarounds becomes a nuisance.