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

Resolver for Mercurial repositories #458

Merged
merged 20 commits into from Sep 20, 2021

Conversation

f-fr
Copy link
Contributor

@f-fr f-fr commented Jan 3, 2021

This is a (first) implementation of a resolver for Mercurial repositories. It is basically a copy of the git resolver with git commands replaced by corresponding hg commands.

It also adds some tests for the hg resolver, which are also a copy of the specs for the git resolver. Because hg knows bookmarks (roughly corresponds to git branches) and named branches (not known to git), tests running on branches are duplicated, once for bookmarks and once for named branches.

Note that the specs take some time to run. The reason is that the test suite runs a lot of hg commands and hg is written in Python. Each command has a certain startup time (of the Python interpreter), which is negligible on the command line but very notable when running a bunch of commands in a test suite.

There is way around this: making use of the hg command server feature. But this requires more modifications to the code. So for the sake of ease of reviewing the changes, I'll put the command server stuff in a separate pull request.

I would be happy to hear your comment ;)

@f-fr
Copy link
Contributor Author

f-fr commented Jan 3, 2021

Of course, the tests fail. One probably needs ci to set up mercurial (the tests work locally for me). Any help would be appreciated

@straight-shoota
Copy link
Member

I've added a commit to install mercurial on circle and travis. Seems the Github action runner already has it installed. Specs are still broken, but not because hg is missing.
They succeed locally, though. So not sure what exactly is wrong in CI. But I'm not familiar with hg at all.

@f-fr
Copy link
Contributor Author

f-fr commented Jan 3, 2021

Thanks a lot. For some reason hg does not seem to create parent directories on clone (it does in my system).

There are still three failing tests but according to the error messages something in wrong with the mercurial installation in these cases.

@f-fr
Copy link
Contributor Author

f-fr commented Jan 4, 2021

Finally all tests pass ;)

I fixed some problems with the setup scripts so they work now

  • missing "sudo" on travis-ci
  • installing python and pip and finally Mercurial as a Python package in github workflows (fixes the build problem on macos, I do not know if there is a better way)

They also revealed some problems with running shell commands on Windows, which are fixed now.

src/resolvers/hg.cr Outdated Show resolved Hide resolved
@f-fr
Copy link
Contributor Author

f-fr commented Jan 4, 2021

I added the changes suggested by @Sija in #459. Note that many of them also apply to GitResolver (which has been used as a template for HgResolver).

@f-fr
Copy link
Contributor Author

f-fr commented Jan 5, 2021

One other question: the method #matches? in GitResolver is never called and actually does not work at all (because it refers to some local variable/method dependency which is not defined). I copied the method to HgResolver but, obviously, it doesn't work either.

What should I do in such cases? Submit a new bug report? A new separate PR that removes the method? Discuss such issues here first?

The point is that I do not know what the purpose of this method is/was. So just removing it might be "the wrong thing" ...

@straight-shoota
Copy link
Member

It's probably just a leftover from a previous refactoring. Since it clearly doesn't work, it should be removed. You could just add a commit here, or post a separate PR.

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Jan 6, 2021

We should also be sure to add a section to SPEC.md like https://github.com/crystal-lang/shards/blob/master/SPEC.md#git.

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.

This is requiring a merge, but besides that it looks awesome. Thanks a lot @f-fr !

@f-fr
Copy link
Contributor Author

f-fr commented Jun 30, 2021

so is there something I should/could do?

@straight-shoota
Copy link
Member

Yes, please merge the master branch, resolving conflicts.

@straight-shoota straight-shoota merged commit 02055f9 into crystal-lang:master Sep 20, 2021
@f-fr f-fr deleted the feature/hg-resolver branch November 15, 2021 22:46
@straight-shoota straight-shoota added this to the v0.16.0 milestone Dec 13, 2021
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.

None yet

4 participants