-
-
Notifications
You must be signed in to change notification settings - Fork 105
Run postinstall for dependencies before dependents #359
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
Run postinstall for dependencies before dependents #359
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good by me. Wouldn't hurt to add an explanation to the comment.
I agree with the change, but we need a spec first. |
The specs don't seem to run correctly on my machine, so I didn't know how I should test it:
Full output
This includes what appears to be the only place this particular class is tested. It makes it hard to know if a spec I wrote would be correct. :-\ |
Did you run make test or make spec? Can't remember which one is it |
Oh, I was running Either way, now that I know how to get a passing run I can try to come up with a spec. Any tips y'all have would be super helpful, since it looks like this is only done in an integration spec. |
I'm not sure this actually fixes the issue. Unless molinillo returns dependencies in some specific order, I see little reasons why the |
@ysbaddaden It looks like molinillo stores dependencies in a hash. First-order dependencies are inserted up front and nested dependencies inserted during resolution. When it collects the packages, it iterates over that hash, which maintains insertion order. Do you have an example of a scenario where iterating in reverse does not fix the problem? |
I'm afraid this doesn't solve the issue. If you have a Actually dependencies form an acyclic graph, so doing a topological sort is possible. Or, a more naive approach would be loop through the dependencies multiple times, skipping those with sub-dependencies still not installed, until all of them are done. Let me know if you want to take care of that. Otherwise, just open an issue and I'll take care of it. |
Oof, you're right. I added
Unfortunately, this PR is the first time I've looked into the internals here and it took 2 hours before I arrived at |
The resolver places dependent shards before dependencies, but
postinstall
script for the dependent shard may depend on thepostinstall
script in its dependencies having been run first.For example, my
grpc
shard depends onprotobuf
, and both of them have executables they install (built viashards build
), but installing them in this order results in this output (theshards
command from the app and theshards build
command in thegrpc
shard are both using-v
here):Iterating from the dependencies first works: