From 3b5350bd1cb66f1fb711217e64a64ee4a994c9ff Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Tue, 26 May 2020 21:59:20 -0700 Subject: [PATCH] fix(builtin): fix linker common path reduction bug where reduced path conflicts with node_modules --- internal/linker/index.js | 9 - internal/linker/link_node_modules.ts | 15 -- .../linker/test/link_node_modules.spec.ts | 241 ++++++++++-------- 3 files changed, 131 insertions(+), 134 deletions(-) diff --git a/internal/linker/index.js b/internal/linker/index.js index a3e6394c99..edc3050970 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -247,15 +247,6 @@ function liftElement(element) { if (link && allElementsAlignUnder(name, link, children)) { return { name, link }; } - if (!link && allElementsAlign(name, children)) { - return { - name, - link: toParentLink(children[0].link), - }; - } - if (children.length === 1 && !link) { - return children[0]; - } return element; } function toParentLink(link) { diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index b0346b72ab..1e7939373b 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -409,21 +409,6 @@ function liftElement(element: LinkerTreeElement): LinkerTreeElement { return {name, link}; } - // No link but all child elements have aligning links - // => the link can be lifted to here - if (!link && allElementsAlign(name, children)) { - return { - name, - link: toParentLink(children[0].link!), - }; - } - - // Only a single child and this element is just a directory (no link) => only need the child link - // Do this last only after trying to lift child links up - if (children.length === 1 && !link) { - return children[0]; - } - return element; } diff --git a/internal/linker/test/link_node_modules.spec.ts b/internal/linker/test/link_node_modules.spec.ts index 0efa722385..4be79132b7 100644 --- a/internal/linker/test/link_node_modules.spec.ts +++ b/internal/linker/test/link_node_modules.spec.ts @@ -47,61 +47,70 @@ describe('link_node_modules', () => { it('should pull aligned child paths up', () => { const IN: LinkerAliases = { + '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], + '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], '@foo/a/1': ['execroot', `${BIN_DIR}/root/sub/a/1`], '@foo/a/2': ['execroot', `${BIN_DIR}/root/sub/a/2`], }; - const OUT: LinkerTreeElement[] = [{ - name: '@foo', - link: ['execroot', `${BIN_DIR}/root/sub`], - }]; - + const OUT: LinkerTreeElement[] = [ + {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} + ]; expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should pull deep aligned child paths up', () => { const IN: LinkerAliases = { + '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], + '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], '@foo/a/b/1': ['execroot', `${BIN_DIR}/root/sub/a/b/1`], '@foo/a/b/2': ['execroot', `${BIN_DIR}/root/sub/a/b/2`], }; - const OUT: LinkerTreeElement[] = [{ - name: '@foo', - link: ['execroot', `${BIN_DIR}/root/sub`], - }]; - + const OUT: LinkerTreeElement[] = [ + {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} + ]; expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should not change aligned paths with a misaligned parent', () => { const IN: LinkerAliases = { + '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], + '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], '@foo/a/b/1': ['execroot', `${BIN_DIR}/root/sub/other/a/b/1`], '@foo/a/b/2': ['execroot', `${BIN_DIR}/root/sub/a/b/2`], }; const OUT: LinkerTreeElement[] = [{ - 'name': '@foo/a/b', - 'children': [ - {'name': '@foo/a/b/1', 'link': ['execroot', `${BIN_DIR}/root/sub/other/a/b/1`]}, - {'name': '@foo/a/b/2', 'link': ['execroot', `${BIN_DIR}/root/sub/a/b/2`]}, - ], + name: '@foo', + children: [{ + name: '@foo/a', + link: ['execroot', `${BIN_DIR}/root/sub/a`], + children: [{ + name: '@foo/a/b', + link: ['execroot', `${BIN_DIR}/root/sub/a/b`], + children: [ + {name: '@foo/a/b/1', link: ['execroot', `${BIN_DIR}/root/sub/other/a/b/1`]}, + {name: '@foo/a/b/2', link: ['execroot', `${BIN_DIR}/root/sub/a/b/2`]} + ] + }] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should not reduce parent/child when parent linking to different path', () => { const IN: LinkerAliases = { '@foo/a': ['execroot', `${BIN_DIR}/root/foo`], + '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], '@foo/a/b/1': ['execroot', `${BIN_DIR}/root/sub/a/b/1`], '@foo/a/b/2': ['execroot', `${BIN_DIR}/root/sub/a/b/2`], }; const OUT: LinkerTreeElement[] = [{ - name: '@foo/a', - link: ['execroot', `${BIN_DIR}/root/foo`], + name: '@foo', children: [{ - name: '@foo/a/b', - link: ['execroot', `${BIN_DIR}/root/sub/a/b`], - }], + name: '@foo/a', + link: ['execroot', `${BIN_DIR}/root/foo`], + children: [{name: '@foo/a/b', link: ['execroot', `${BIN_DIR}/root/sub/a/b`]}] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); @@ -111,11 +120,9 @@ describe('link_node_modules', () => { '@foo/a/1': ['execroot', `${BIN_DIR}/root/sub/a/1`], '@foo/a/2': ['execroot', `${BIN_DIR}/root/sub/a/2`], }; - const OUT: LinkerTreeElement[] = [{ - name: '@foo', - link: ['execroot', `${BIN_DIR}/root/sub`], - }]; - + const OUT: LinkerTreeElement[] = [ + {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} + ]; expect(linker.reduceModules(IN)).toEqual(OUT); }); @@ -125,51 +132,47 @@ describe('link_node_modules', () => { '@foo/a/1': ['runfiles', `${BIN_DIR}/root/sub/a/1`], }; const OUT: LinkerTreeElement[] = [{ - name: '@foo/a', - link: ['execroot', `${BIN_DIR}/root/sub/a`], - children: [ - { - name: '@foo/a/1', - link: ['runfiles', `${BIN_DIR}/root/sub/a/1`], - }, - ], + name: '@foo', + children: [{ + name: '@foo/a', + link: ['execroot', `${BIN_DIR}/root/sub/a`], + children: [{name: '@foo/a/1', link: ['runfiles', `${BIN_DIR}/root/sub/a/1`]}] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should not reduce sibling aliases with different link types', () => { const IN: LinkerAliases = { + '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], '@foo/a/1': ['runfiles', `${BIN_DIR}/root/sub/a/1`], '@foo/a/2': ['execroot', `${BIN_DIR}/root/sub/a/2`], }; const OUT: LinkerTreeElement[] = [{ - name: '@foo/a', - children: [ - { - name: '@foo/a/1', - link: ['runfiles', `${BIN_DIR}/root/sub/a/1`], - }, - { - name: '@foo/a/2', - link: ['execroot', `${BIN_DIR}/root/sub/a/2`], - }, - ], + name: '@foo', + children: [{ + name: '@foo/a', + link: ['execroot', `${BIN_DIR}/root/sub/a`], + children: [ + {name: '@foo/a/1', link: ['runfiles', `${BIN_DIR}/root/sub/a/1`]}, + {name: '@foo/a/2', link: ['execroot', `${BIN_DIR}/root/sub/a/2`]} + ] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should reduce deeply-aligned siblings', () => { const IN: LinkerAliases = { + '@foo/a': ['execroot', `${BIN_DIR}/root/sub/a`], + '@foo/a/b': ['execroot', `${BIN_DIR}/root/sub/a/b`], + '@foo/a/b/c': ['execroot', `${BIN_DIR}/root/sub/a/b/c`], '@foo/a/b/c/d1': ['execroot', `${BIN_DIR}/root/sub/a/b/c/d1`], '@foo/a/b/c/d2': ['execroot', `${BIN_DIR}/root/sub/a/b/c/d2`], }; - const OUT: LinkerTreeElement[] = [{ - name: '@foo', - link: ['execroot', `${BIN_DIR}/root/sub`], - }]; - + const OUT: LinkerTreeElement[] = [ + {name: '@foo', children: [{name: '@foo/a', link: ['execroot', `${BIN_DIR}/root/sub/a`]}]} + ]; expect(linker.reduceModules(IN)).toEqual(OUT); }); @@ -192,59 +195,63 @@ describe('link_node_modules', () => { it('should not reduce aligned paths when link has extra dir', () => { const IN: LinkerAliases = { + '@foo/lib': ['execroot', `${BIN_DIR}/path/to/lib`], '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/noseeme/a`], '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/noseeme/b`], '@foo/lib/c': ['execroot', `${BIN_DIR}/path/to/lib/noseeme/c`], }; const OUT: LinkerTreeElement[] = [{ - name: '@foo/lib', - link: ['execroot', `${BIN_DIR}/path/to/lib/noseeme`], + name: '@foo', + children: [{ + name: '@foo/lib', + link: ['execroot', `${BIN_DIR}/path/to/lib`], + children: [ + {name: '@foo/lib/a', link: ['execroot', `${BIN_DIR}/path/to/lib/noseeme/a`]}, + {name: '@foo/lib/b', link: ['execroot', `${BIN_DIR}/path/to/lib/noseeme/b`]}, + {name: '@foo/lib/c', link: ['execroot', `${BIN_DIR}/path/to/lib/noseeme/c`]} + ] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should not reduce sibling mappings with inconsistent paths', () => { const IN: LinkerAliases = { + '@foo/lib': ['execroot', `${BIN_DIR}/path/to/lib`], '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/x`], '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], }; const OUT: LinkerTreeElement[] = [{ - name: '@foo/lib', - children: [ - { - name: '@foo/lib/a', - link: ['execroot', `${BIN_DIR}/path/to/lib/x`], - }, - { - name: '@foo/lib/b', - link: ['execroot', `${BIN_DIR}/path/to/lib/b`], - }, - ], + name: '@foo', + children: [{ + name: '@foo/lib', + link: ['execroot', `${BIN_DIR}/path/to/lib`], + children: [ + {name: '@foo/lib/a', link: ['execroot', `${BIN_DIR}/path/to/lib/x`]}, + {name: '@foo/lib/b', link: ['execroot', `${BIN_DIR}/path/to/lib/b`]} + ] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); it('should not reduce sibling mappings with inconsistent path parents', () => { const IN: LinkerAliases = { + '@foo/lib': ['execroot', `${BIN_DIR}/path/to/lib`], '@foo/lib/a': ['execroot', `${BIN_DIR}/path/to/lib/x/a`], '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], }; const OUT: LinkerTreeElement[] = [{ - name: '@foo/lib', - children: [ - { - name: '@foo/lib/a', - link: ['execroot', `${BIN_DIR}/path/to/lib/x/a`], - }, - { - name: '@foo/lib/b', - link: ['execroot', `${BIN_DIR}/path/to/lib/b`], - }, - ], + name: '@foo', + children: [{ + name: '@foo/lib', + link: ['execroot', `${BIN_DIR}/path/to/lib`], + children: [ + {name: '@foo/lib/a', link: ['execroot', `${BIN_DIR}/path/to/lib/x/a`]}, + {name: '@foo/lib/b', link: ['execroot', `${BIN_DIR}/path/to/lib/b`]} + ] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); @@ -255,20 +262,31 @@ describe('link_node_modules', () => { '@foo/lib/b': ['execroot', `${BIN_DIR}/path/to/lib/b`], }; const OUT: LinkerTreeElement[] = [{ - name: '@foo/lib', - link: ['execroot', `${BIN_DIR}/path/to/other/lib`], - children: [ - { - name: '@foo/lib/a', - link: ['execroot', `${BIN_DIR}/path/to/lib/a`], - }, - { - name: '@foo/lib/b', - link: ['execroot', `${BIN_DIR}/path/to/lib/b`], - }, - ], + name: '@foo', + children: [{ + name: '@foo/lib', + link: [ + 'execroot', + `${BIN_DIR}/path/to/other/lib`, + ], + children: [ + { + name: '@foo/lib/a', + link: [ + 'execroot', + `${BIN_DIR}/path/to/lib/a`, + ] + }, + { + name: '@foo/lib/b', + link: [ + 'execroot', + `${BIN_DIR}/path/to/lib/b`, + ] + } + ] + }] }]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); @@ -296,31 +314,35 @@ describe('link_node_modules', () => { name: '@foo/c/c', link: ['execroot', `${BIN_DIR}/path/to/foo_cc`], children: [{ - name: '@foo/c/c/c/c', - link: ['execroot', `${BIN_DIR}/path/to/foo_cccc`], + name: '@foo/c/c/c', + children: [ + {name: '@foo/c/c/c/c', link: ['execroot', `${BIN_DIR}/path/to/foo_cccc`]} + ] }] }] }, { name: '@foo/d', - link: ['execroot', `${BIN_DIR}/path/to/foo_d`], - }, - ], - }, - { - name: 'a', - link: ['execroot', `path/to/lib_a`], + children: [{ + name: '@foo/d/bar', + link: ['execroot', `${BIN_DIR}/path/to/foo_d/bar`], + children: [{ + name: '@foo/d/bar/fum', + children: [{ + name: '@foo/d/bar/fum/far', + link: ['execroot', `${BIN_DIR}/path/to/foo_d/bar/fum/far`] + }] + }] + }] + } + ] }, - { + {name: 'a', link: ['execroot', 'path/to/lib_a']}, { name: 'b', link: ['execroot', 'external/other_wksp/path/to/lib_b'], - children: [{ - name: 'b/b', - link: ['execroot', 'external/other_wksp/path/to/lib_bb'], - }], + children: [{name: 'b/b', link: ['execroot', 'external/other_wksp/path/to/lib_bb']}] } ]; - expect(linker.reduceModules(IN)).toEqual(OUT); }); }); @@ -390,8 +412,7 @@ describe('link_node_modules', () => { '@foo/c/c': ['execroot', `${BIN_DIR}/path/to/foo_cc`], '@foo/d/bar/fum/far': ['execroot', `${BIN_DIR}/path/to/foo_d/bar/fum/far`], '@foo/d/bar': ['execroot', `${BIN_DIR}/path/to/foo_d/bar`], - // don't include `@foo/d` as the linker should derive that symlink - // from the lowest common denominator of the module name & module path + '@foo/d': ['execroot', `${BIN_DIR}/path/to/foo_d`], }, 'workspace': workspace, });