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

Add fossil resolver #530

Merged

Conversation

MistressRemilia
Copy link
Contributor

@MistressRemilia MistressRemilia commented Oct 28, 2021

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
Copy link

chillfox commented Jan 8, 2022

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

This is necessary for projects that are themselves under Fossil.
@chillfox
Copy link

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
Copy link
Contributor Author

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

MistressRemilia and others added 9 commits January 31, 2022 02:46
Versions before 2.12 did not have a --workdir argument for the
timeline command, and were more strict about argument ordering.  This
can be faked using Dir#cd and arguments in the correct order.

Versions before 2.14 did not support the --format/-F argument for the
timeline command.  As a workaround, we can use timeline + whatis to
get the full artifact hash.
.github/workflows/ci.yml Outdated Show resolved Hide resolved
docs/shard.yml.adoc Outdated Show resolved Hide resolved
end

retLines.reject! &.nil?
[/artifact:\s+(.+)/.match(run("fossil whatis #{retLines[0]}")).not_nil!.[1]]
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it returns a list with only a single commit instead of a list of all commits. What's the reason for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That mirrors the behavior of the Mercurial code, which this was based on. That code looks like this:

run("hg log --template=#{Process.quote("{node}\n")} -r #{Process.quote(rev)}").strip.split('\n')

Let me go ahead and adjust the Fossil-based stuff to not rely on a list since it seems like it's not needed.

@straight-shoota
Copy link
Member

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
Copy link

chillfox commented Feb 7, 2022

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.

Co-authored-by: Johannes Müller <straightshoota@gmail.com>
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
@MistressRemilia
Copy link
Contributor Author

MistressRemilia commented Feb 9, 2022

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
Copy link
Member

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.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

LGTM too 🚀
Many thanks @MistressRemilia 🙇 🙇

@straight-shoota straight-shoota added this to the v0.17.0 milestone Mar 8, 2022
@straight-shoota straight-shoota merged commit ffef6b9 into crystal-lang:master Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Fossil
4 participants