Fix transient bin links overriding direct ones #5012
Conversation
If a transient dependency has a bin link with the same name as a direct dependency, sometimes the top level bin link with lead to the transient dependency rather than the direct one after install. This test asserts that bin links from direct dependencies are installed.
The bin links are created in two passes during install. In the first pass all direct dependencies for the installing module and transient dependencies are created in their respective bin directories. In the second pass top level bin links are created for all modules, including transient dependencies. This patch makes sure the second pass doesn't overwrite links from the first pass which fixes a problem where links from transient dependencies could overwrite links from direct dependencies.
There was a problem hiding this comment.
This new test only passes because your direct deps a-dep and b-dep are alphabetically sorted before the transitive c-dep and d-dep.
If you rename a-dep to z-dep then the test will fail (z-dep will be bin linked later than it's transitive deps now, and so the bin link will exist already and won't be overwritten)
Also, I mess this up all the time myself too, but the correct term is "transitive" not "transient", it would be nice to fix that term where it shows up in code. Some of them are probably my fault 😞
My own understanding of how these bin links were made was wrong after I actually stepped through the code. I was mistaking "top level deps" for "direct deps" in my own thought process.
It seems like before we loop over the packages to link, we should re-order them not just alphabetically as they are now, but by non-direct (transitive) first, then direct deps second (which is what I thought it actually did, but apparently does not).
|
Unfortunately I can't push a commit to this, but I pushed a commit to my fork of Yarn that would fix this issue. @AndersAstrand ~~~have a look at rally25rs@c471ccf and see what you think. We could either incorporate those changes to this PR, or I could PR my branch and close this one (I'd hate to hijack your PR).~~~ Edit: I figured out that I can just submit a PR from my fork to yours: https://github.com/AndersAstrand/yarn/pull/1 Thank you for writing the test case for this, it really helps! 👍 |
|
Thanks a lot for your comments! Transitive, got it, I read transient somewhere I think and just went with it :). You are, of course, correct about the I suspected my proposed fix was way too naive to actually work, but a green test run have a tendency to instill confidence :). Feel free to just close this pull request or hi-jack it completely. I'm only happy if I could be of any help in getting this bug fixed since it's what's stopping us from switching to yarn. |
Trust me, I do the same thing myself all the time 😆
Closing this PR and opening #5016 to replace it (it's the same as this PR with 1 additional commit)
And we thank you for your contribution! 🎉 The more community members we can get involved, the better this project will be 👍 |
Summary
This fixes #4937
The problem still exists when doing
yarn addin an already installed module however, but that is unaffected by this patch as far as I can tell, and all tests pass.This is my very first foray into the yarn code base, so please feel free to comment on anything in here. However petty it might seem to you!
Test plan
Run the test suite.