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
Mercurial Resolver for Bower #1448
Conversation
Bower Mercurial resolver
Very cool! I'm not familiar w/ Mercurial, but just want to say thanks for the PR and the hard work! |
@sindresorhus Can this go to 1.4.x? |
Yes. Up to you :) |
Any update on this PR? |
Someone needs to review this. Currently I'm working on other bower stuff. |
@sheerun; I'd be willing to test this PR in an actual environment, would that be of any help? |
yes :) just confirmation everything works as expected at least on short run |
Last week I've tested this version of Bower, specifically TradeMe/bower@b64b365. I've elaborated my findings in the following table:
Overall, Bower supporting Mercurial in general is worth the inability to resolve to a specific commit in my opinion. |
Any clue why you cannot resolve specific commit? Is there a reason? |
When specifying a commit like this:
Bower reports the following:
Considering the fact that "Tag/branch" is mentioned specifically implicates the lack of commit resolving is intended. Also, when looking at the source code and unit tests the commit resolving mechanism just seems to be absent. |
There seems to be commit detection: https://github.com/TradeMe/bower/blob/master/lib/core/resolvers/HgResolver.js#L158-L165 It works incorrectly then. @phenomnomnominal Could you fix it? |
I think bower shoudn't support revision number, but only changeset ids. AFAIK using revision numbers is not deterministic? |
It should only support changeset ID's indeed. Apparently I've missed that feature.. Looking at the regular expression ( The prefixing feels off though, especially since it's not the way to go for Git repositories on both Bower and NPM. |
Happy to make any changes needed, what would the expected behaviour be? |
Recognize commits as |
Plus maybe some tests |
We are about to start a private bower registry and need to have Mercurial behind it. Any chance I can help getting this pull request into the master in the near future? Thx. |
@sheerun, the tests seem to be pretty complete actually, so I went ahead with adjusting the commit resolving. As the pull request has gotten pretty outdated I tried to merge the most recent version of Bower into it. Unfortunately, this caused the following unit tests to fail:
@phenomnomnominal, could you please fix this? You can check out my fork to see the issues directly. |
* Merged in latest bower * Update unit tests with changes * Fixed revision ID matching RegExps to not require a leading ‘r’
@RobbinHabermehl think I've got this all sorted if someone wants to have a look. |
@phenomnomnominal, I just checked out your updates and ran the tests. Unfortunately four unit tests are still failing, the good news is that it's one less than last time. Also, the cause of the failures has changed, there appear to be some difficulties running I've posted the stack traces below:
|
Hmm that's bizarre, they all pass on my machine. I'll investigate further! |
Perhaps it has to do with the OS, Node.js or Mercurial? I'm running Mac OS X 10.10, Node.js is running version 0.12.0 and my Mercurial version is 3.1.1. The Node.js dependencies are up to date as I performed a fresh install. |
@RobbinHabermehl Yeah it might be something, although I just ran them on my Windows box at work and they all passed (apart from some breaking due to different new line characters). I'll try updating my mercurial at home and having another go. |
I just updated Mercurial to version 3.3 and the number of failing tests has been reduced a little further, now only three tests are failing:
Apparently something causes Mercurial to exit with status code 255. |
Still can't repro this, tried a few machines... |
I tried removing the repository locally and cloning it again, but then I'm still encountering these three failing tests. In order to determine whether the problems are caused by my MacBook I ran the tests on another machine (OS X 10.10.2, Node.js v0.12.0, NPM v2.5.1 and Mercurial v3.3) as well, unfortunately resulting in the same errors. As this didn't help I started to dig deeper and started to analyze the
Due to the order in the console it seems that no errors are related to the third failing test. Mocha's stack trace shows the cause is Two of the logs clearly state the |
Awesome, will look into this tonight! |
I have no idea what this is. Can't reproduce, have cloned it on every computer I have access to. Stumped. |
Which OS's have you tried @phenomnomnominal? And which version of Node.js was installed? I just noticed Bower is optimized for Node.js v0.10.0, while I've been running v0.12.0 on both devices. Perhaps that's the cause. I guess that appveyor.yml should be extended with a |
Tried it on Windows 7 and OS X 10.10, both with 0.10.36. I'm not sure if the node version would be the problem, because node is basically just running Mercurial. It must be an issue with how Mercurial is running. Frustrating. |
Later today I'll run the integration tests again to determine whether this PR can finally be merged. |
As promised, hereby the results of my integration tests:
As you can see all of these scenarios have passed now. @sheerun, could you take a look at my previous question and let us know how to handle that? Once that's been taken care of, this pull request seems ready to be merged! |
@phenomnomnominal I have time to look at it. Could you rebase it on master? There were many changes there since this PR (including many more tests to possibly fail). We need passing tests on all platforms (Linux, OSX on Travis and Windows on Appveyor). |
Also, please fix indentation and whitespacing (tabs instead of 4 spaces etc.) :) |
Any news on this? I would realy like to see Mercurial support in bower. |
Yes, pluggable revolvers I'm working on will allow it. On Wednesday, July 15, 2015, renegat4 notifications@github.com wrote:
|
Sorry, this thing has fallen off my radar a bit as my current project is no longer using bower. I will hopefully have some free time to get this PR sorted and up to date it if is still even required? |
Yes, that would be awesome @phenomnomnominal. We use mercurial in our company and we're struggling to find a reasonable option to handle common, non-public front-end libraries. Let me know if I can help you with anything. |
@phenomnomnominal I managed to implement Pluggable Resolvers interface for Bower. It has different (simpler) API than native resolvers, so some refactor is needed to implement Mercurial resolver, but now you can publish it by yourself :) No need to wait for anyone to review. I really appreciate the work you've done! I hope you'd find time to re-write it as Pluggable Resolver. The plan is to re-write all native resolvers as plugin resolvers, and switch to them in Bower 2.0. I'm looking forward to to include Mercurial as one of the built-in pluggable resolvers. |
It's good news! |
I'm having a look at this now, will see how I get on in the next couple of days. Repo will be here: https://github.com/phenomnomnominal/mercurial-bower-resolver |
https://github.com/phenomnomnominal/mercurial-bower-resolver is pretty much working now, please try it and open issues! |
is the intention still to ship the mercurial resolver plug-in as one of the build-in resolvers? |
Seems to be working! Resolver for ssh/http/https Mercurial repositories + tests.