fix: allow multiple copy_file in the same package

[alexeagle]

Previously this failed on windows, as we wrote colliding batch files

[alexeagle]

Ping @tetromino

[alexeagle]

note: users of the public API wouldn't observe a problem because only one declare_file happens per copy_file rule, and so ctx.label.name is sufficient to disambiguate.

We hit this in a scenario where a rule uses this library code and creates multiple actions.

[alexeagle]

note: users of the public API wouldn't observe a problem because only one declare_file happens per copy_file rule, and so ctx.label.name is sufficient to disambiguate.

We hit this in a scenario where a rule uses this library code and creates multiple actions.

[tetromino]

I might be blind here, but I don't think I understand the purpose of this change.

As far as I can see, two copy_file targets (even with the same src) in the same package shouldn't collide on Windows because they will have different labels.

It seems the fix is only for a hypothetical situation where another rule's implementation in Skylib might call copy_cmd in a loop - but no such rule exists at the moment (again, as far as I can see).

Is this a prerequisite for another PR - if so, which one? I don't think that #323 should need this...

[alexeagle]

Yes I mentioned it in #324 (comment)
the particular rule in this case is https://github.com/bazelbuild/rules_nodejs/blob/stable/internal/js_library/js_library.bzl#L34-L38

Granted, that rule uses the private API of copy_file and therefore there's no support guarantee for doing this. You could just say no. However I think it's nice to fix it anyway.

[tetromino]

I'm wondering if we should instead bite the bullet and fix run_shell to support .bat and powershell commands. Then we wouldn't need to write temporary bat files at all, here and elsewhere.

Wrote up my thoughts in bazelbuild/bazel#15194

[alexeagle]

That looks interesting, but it's totally orthogonal to this fix, right?

[tetromino]

If run_shell supported command_bat, we wouldn't need copy_cmd and copy_bat at all: the implementation would be unified into

    args = ctx.actions.args()
    args.add_all(src.path, dst.path)
    ctx.actions.run_shell(
        tools = [src],
        outputs = [dst],
        command = "cp -f \"$1\" \"$2\"",
        command_bat = "copy /Y \"%1\" \"%2\" >NUL",
        arguments = [args],
        mnemonic = "CopyFile",
        progress_message = "Copying files",
        use_default_shell_env = True,
    )