Transitive dependencies are conflicting

[lexor90]

Hey there, After upgrading to newer versions of bazel I started using this rules (previously I was using an in-house version that had similiar concepts to this library before the latest major rewrite - no package.json support -).

Today I added these rules and started rewriting the repository to use these ones, and soon I faced two issues:

1. was already ticketed in #37. Specifically, we have tests in a each project's subdirectory "tests/". Specifying the main = "tests/index-test.js" does not work because of the way the mocha_tests rule declares the entrypoint (it uses the rule name). I've seen you patched it using PACKAGE_NAME, but, correct me if I'm wrong this would require a BUILD file inside of the tests/ directory - side note: furthermore PACKAGE_NAME is now deprecated in favor of package_name() -. It was not comfortable for me to add several thousand of new build files, so I simply patched the mocha_test rule by adding:

   if len(main.split("/")) > 1: for directory in main.split("/")[:-1]: entrypoint.append(directory)

This way let's assume the rule is:

mocha_test(
    name = "test",
    main = "tests/routes-test.js",
    data = glob(["**/*"]) + ["//promotely/keys"],
    deps = [
      "//lib/mylib",
      "@npm_async//:_all_",
      "@npm_cors//:_all_",
    ]
)

It now properly appends to entrypoint the tests/ path (previously it was referring to the test_module root path, resulting in mocha unable to find tests) finding the test files correctly.

Maybe there are better ways to do this, but I didn't want to rewrite the full rule (moreover I'm still getting used to the new APIs coming with Bazel >= 6). This has been tested with bazel 6 and bazel 8.

2. Transitive dependencies conflicting

I don't know if this also affects previous versions, but since the creation of the node_modules() rules, if you declare multiple packages (mixing node_module and yarn_modules) that have some npm/yarn package in common (but with different versions) bazel stops because of file 'output/project/test_modules/copy_library.sh' is generated by these conflicting actions. I understand that this is one of Bazel's principles (to avoid diamond dependencies) and while I perfectly agree in the open-source world this is very difficult to achieve.

I tracked it down and applied a quick fix, in order to continue the integration, but I'm pretty unsure about the best thing to do here. Right now I'm filtering transitive dependencies in node_modules.bzl's _copy_modules() by their identifier. This is the modified version:

def _copy_modules(ctx, target_dir, deps):
    outputs = []
    transitive_deps = {}

    for dep in deps:
        module = dep.node_module
        outputs += _copy_module(ctx, target_dir, module)
        for module in module.transitive_deps:
            transitive_deps[module.identifier] = module
    
    # print(transitive_deps.keys())
    for module in transitive_deps.values():
        outputs += _copy_module(ctx, target_dir, module)

    return outputs

This way we do only produce a single copy_library.sh per identifier. But this also comes with problems: different versions. Each yarn module uses its own version of a library, and while a minor upgrade might be pretty safe to assume to be working, a major update would be scaring. In my case, several packages rely on the hoek package with versions ranging from 2.x to 4.x. And my patch currently copies only one of them (which one is undefined). While tests might get me covered I don't think this is a proper solution. I was thinking to rename the "copy_X.sh" to something including the version name, but this would not fix the final result (node_modules will contain "hoek"), while adding the version also to the library itself does break the require lookup -of course-).

What are your thoughts about it? Maybe applying some concept such as nested node_modules could work, but I don't know if there are other downsides to this approach. Right now it seems to me to be the best solution.

P.S. feel free to rename this issue, I could not think a better title. Also should you prefer I could split the two issues into two separated tickets.

[lexor90]

I tried to gain a better understanding of what's going on here, and it seems to me that the conflict for the issue n.2 arises because all the transitive dependencies are flatted up to the module level.

So if my project, say app depends on lib1 and lib2, where:

app
  ---- [email protected]
       ---- [email protected]
  ---- [email protected]
       ---- [email protected]

It flattens dependencies this way (correct me if I'm wrong):

 app
  ---- [email protected]
  ---- [email protected]
  ---- [email protected]
  ---- [email protected] -> issue for duplicated output "node_modules/app/dep"

This happens during the _copy_module action because I can see that the original files get properly installed (also confirmed by the fact that we are not getting this issue with yarn) - example with a real dependency "cors" -

cp 'bazel-out/darwin-fastbuild/bin/external/npm_cors/vary/LICENSE' 'bazel-out/darwin-fastbuild/bin/apps/admanager/test_modules/node_modules/vary/LICENSE'

You can see it looses the nesting, while the correct format should be: cp 'bazel-out/darwin-fastbuild/bin/external/npm_cors/vary/LICENSE' 'bazel-out/darwin-fastbuild/bin/apps/admanager/test_modules/node_modules/ cors/ vary/LICENSE'.

I was figuring out how to make this, but it seems to me that transitive dependencies are flatted up during the node_module declaration (which mocha_test uses internally), and we're losing the context of the parent (who created that dependency), but probably I'm missing something else here.

Do you have any idea? To give more context, I'm getting this issue with a combination of request, jsonwebtoken and other deps, which also depend on jsonwebtoken and the dependency giving issues is hoek.

+friendly ping @pcj

[pcj]

Sorry @lexor90 this has not recvd the attention it deserves! I hope to look at this in more detail but don't have a time estimate.