fix(builtin): link module_name to directories recursively to avoid directory clashes #1432
Conversation
jbedard
requested review from
alexeagle,
gregmagolan and
soldair
as code owners
jbedard
force-pushed
the
overlapping-module_name
branch
from
b7e5507 to
006a844
Compare
|
Nice! Thanks for taking a look at this @jbedard . I'll TAL on Friday after BazelCon. |
| .toEqual('/*@foo/d/bar/fum/far*/exports = {}'); | ||
| }); | ||
|
|
||
| it('should handle first-party packages with sibling directories', async () => { |
There was a problem hiding this comment.
These tests talking to the real FS are slower. It seems like you add all the same coverage below as in the IN/OUT unit tests above. Do we really need both? Do these new tests below add coverage that we don't get from the ones above?
There was a problem hiding this comment.
They go one step further and test the use of that OUT tree, concurrently traversing it and applying it to the FS.
They could also be run with both the old and new code so I could assure I'm actually fixing things, the test failed before and passes now etc.
We could potentially remove a few now that we're (I hope) more confident everything is working. Only keep the ones that concentrate more on applying the tree to the FS...
…rectory clashes Fixes bazel-contrib#1411
jbedard
force-pushed
the
overlapping-module_name
branch
from
006a844 to
234a959
Compare
…void directory clashes Update comments/formatting
…void directory clashes ensure all FS test cases have equivelent reduceModules cases
…void directory clashes update fs module-name tests to test unique link fs-layouts, not unique module_name=>link conversions
|
I've updated this with a couple changes:
I've left those as fixup commits for now so you can see the changes. If we're good I or whoever can squash+merge. |
Fixes #1411
The overall change is to build a recursive hierarchy instead of just an array of arrays (one level of the hierarchy?). That hierarchy is then the order changes must be processed in, siblings in the hierarchy can be processed concurrently as long as the parent was already processed.
IDK if such a drastic change was really necessary, but it's the path I started down. Maybe we can just adjust the existing implementation but keep the tests?
This also does a bit less
mkdirpcalls, the@foo/c/c/c/ctest now has I think 17 mkdirp/symlink calls instead of 21 🤷♂2 of the tests fail before the change, I might try finding more cases though...5 of the tests fail before the change, I think all of which are ~unique situations. I've outlined these more in the issue.