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

Use Molinillo to resolve dependencies #322

Merged
merged 14 commits into from
Feb 12, 2020
Merged

Use Molinillo to resolve dependencies #322

merged 14 commits into from
Feb 12, 2020

Conversation

waj
Copy link
Member

@waj waj commented Feb 7, 2020

This PR integrates a port of the Molinillo dependency solver.

I tried to keep the PR minimal, maintaining the semantics of the current commands. Only the error reporting is different.

Fixes #303

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Can we remove all the remaining commented out debug code?

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

That's incredible. Integration is smooth, and you got all tests passing without tweaking any of them!

I don't have much to say. Maybe just do some cleanup. For example drop the SAT solver which is now dead code. It's not a particularly elegant or efficient one, and we can still find it in the Git history. Also, a specialized solver will always beat a generic SAT solver.

Maybe tag a release for molinillo, too.

@asterite
Copy link
Member

Since nobody mentioned it yet it's probably not a concern, but... is it okay that shards now has a dependency? I'm thinking of how we distribute Crystal, distributions now must install an old shards binary as part of the process.

@jhass
Copy link
Member

jhass commented Feb 10, 2020

An alternative to requiring an old shards binary is to provide release tarballs for shards that vendor in the dependencies. Basically tar up the result, minus any .gitdirectories, of git clone shards, git checkout tag, shards install.

@waj
Copy link
Member Author

waj commented Feb 10, 2020

@Sija @RX14 i just did the requested changes
@ysbaddaden I removed the SAT solver, but please at least create a separate shard with it! :)

@waj
Copy link
Member Author

waj commented Feb 10, 2020

Regarding the shard dependency, last resort would be checking out molinillo code into lib/molinillo manually before the build

@waj
Copy link
Member Author

waj commented Feb 10, 2020

Maybe tag a release for molinillo, too.

@ysbaddaden I was thinking we could tag molinillo once we did enough testing

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

I am happy with the current state. Manual checkout can work we can have a nightlies of 0.34 to use this already and encourage early testing. Before releasing 0.34 we can tag molinillo and shards 0.10 if everything goes fine.

I was also happy to use git-submodules but as long as it is a single dependency there is no problem to do the checkout.

Maybe we can have a make lib/molinillo target to do the checkout, specially for package maintainers.

I thought there was class that you wanted to move from shards to molinillo @waj. I don't distinguish or recall what happen with that.

The other story I remember we discussed was how theck if the lock was valid for the current spec in the case of a ref in lock and branch in spec. I am missing if there was a rule to check for reachability of the lock from the branch to decide whether is valid or not. If so, maybe some comment around is missing :-)

@ysbaddaden
Copy link
Contributor

Or add this Ruby-based bootstrap that will read shard.lock and install dependencies at their locked versions.

@RX14
Copy link
Contributor

RX14 commented Feb 11, 2020

I'd rather just vendor or move the molinillo resolver into shards. It shouldn't depend on itself. You loose the benefits of having it be a shard, but, it's probably not worth the extra complexity.

@RX14
Copy link
Contributor

RX14 commented Feb 11, 2020

Also @ysbaddaden, that bootstrap is brittle since github refuses to allow you to download any sha1 which is not referred to by a git ref. For a bootstrap you probably have to --mirror.

A shards bootstrap doesn't have to not depend on crystal, it can require the whole of shards except the resolver.

@waj waj merged commit 38b3d21 into master Feb 12, 2020
@waj waj deleted the molinillo branch February 12, 2020 19:23
@waj
Copy link
Member Author

waj commented Feb 12, 2020

I just merged this before it starts diverging from master too much. We can continue the discussion about how to integrate better with the molinillo shard.

@RX14
Copy link
Contributor

RX14 commented Feb 13, 2020

I think that distributing a tarball with shards "pre-run" for distros is fine.

@j8r
Copy link
Contributor

j8r commented Feb 13, 2020

Another option is to use git subtree / submodule to include molinillo inside shards, without having to use shards.
It would essentially be vendoring, but also keep history.

@anatol
Copy link
Contributor

anatol commented Apr 7, 2020

I am trying to build the latest shards release 0.10.0 and when I do make CRFLAGS=--release the build fails with

find: ‘lib/molinillo’: No such file or directory
crystal build src/shards.cr -o bin/shards --release
Showing last frame. Use --error-trace for full trace.

In src/molinillo_solver.cr:1:1

 1 | require "molinillo"
     ^
Error: can't find file 'molinillo'

If you're trying to require a shard:
- Did you remember to run `shards install`?
- Did you make sure you're running the compiler in the same directory as your shard.yml?
make: *** [Makefile:23: bin/shards] Error 1

Could you please clarify what is the correct set of the build instructions?

@Sija
Copy link
Contributor

Sija commented Apr 7, 2020

@anatol It seems you haven't run shards install beforehand (as noted in the error message).

@straight-shoota
Copy link
Member

straight-shoota commented Apr 7, 2020

You need to install molinillo into ./lib/molinillo folder. If you already have a shards executable available, you can just run shards install in the root directory. Otherwise curl https://github.com/crystal-lang/crystal-molinillo/archive/v0.1.0.tar.gz | tar -xf - -C lib/molinillo should do.
We should probably add a target to the Makefile.

@RX14
Copy link
Contributor

RX14 commented Apr 7, 2020

We should add a bootstrap target to to the makefile so you don't need shards to compile shards.

@RX14
Copy link
Contributor

RX14 commented Apr 7, 2020

@anatol for the PKGBUILD, downloading https://github.com/crystal-lang/crystal-molinillo/archive/v0.1.0.tar.gz and then mv "$srcdir"/crystal-molinillo-0.1.0 "$srcdir"/shards-$pkgver/lib/molinillo should be your best bet.

@straight-shoota
Copy link
Member

I've added a bootstrap to #344

taylor pushed a commit to vulk/shards that referenced this pull request Aug 11, 2020
f-fr pushed a commit to f-fr/shards that referenced this pull request Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shards takes hours to resolve dependencies
10 participants