if resource_bundles has exclude_directories = false, clonefile fails

[taowang487]

Bugfix for this PR: https://github.com/bazelbuild/rules_apple/commit/5401fe26c813c0ec0ae8b0e0a00759215c90be43.

before this PR, we are using cp -c, which will overide the destination file if exists. However, the clonefile will just fail if the destination exists.

An alternative is to revert to shutil.copy(src, full_dest).

Did some benchmark for copying a tiny file 10000 times in my machine:

• subprocess.check_output(["/bin/cp", "-c", src, dest]): 46 seconds
• subprocess .Popen(['cp', 'x', 'y'], stdout=DEVNULL, stderr=STDOUT): 55 seconds
• clonfile: 1.58 seconds
• check exists + remove + clone file: 2.14 seconds
• shutil.copy: 4.05 seconds

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[thii]

Can you sign a CLA?

[brentleyjones]

We don't want to revert to shutil.copy, because it's not all about the time savings, it's also about the space savings.

[taowang487]

Can you sign a CLA?

yes, I just signed the CLA, maybe we need some time for it to be populated.

[brentleyjones]

How does the file already exist? Bazel always starts with a fresh bundle.

[taowang487]

How does the file already exist? Bazel always starts with a fresh bundle.

I am not sure why this would happen. I can reproduce this consistently in my App for both incremental/clean builds. Let me check if I can find something useful.

[taowang487]

How does the file already exist? Bazel always starts with a fresh bundle.

I am not sure why this would happen. I can reproduce this consistently in my App for both incremental/clean builds. Let me check if I can find something useful.

I took a look but not sure where the duplicates come from, maybe we are using globs to include everything? If everything is created from fresh, maybe we can do a no-op instead of remove + clone?

[taowang487]

@brentleyjones One of my teammates helped to take a look today, in his words:

Somehow the bundle_merge_files is returning both foo.lproj and the string in it bar.strings. So the first clonefile of the lproj dir would include the strings file, hence when cloning the individual file caused the error. It seems to be related to how we write the globs for the resource bundle. In podpsec it has s.resource_bundle = { 'XXX' => [ '**/*' ] }

This could be a bug for rules-ios not covering this case.

[ob]

How does the file already exist? Bazel always starts with a fresh bundle.

The problem is that resource_bundles is not excluding directories:

    resource_bundles = {"FooResources": glob(
        ["Resources/**/*"],
        exclude_directories = 0,
    )},

I left a comment on the rules_ios repo asking why we need this and how it works here.

The problem is not that files exist, it's that if you have both dir and dir/file in the inputs copy the dir first and then when we get to the file it's already there.

[taowang487]

How does the file already exist? Bazel always starts with a fresh bundle.

The problem is that resource_bundles is not excluding directories:

    resource_bundles = {"FooResources": glob(
        ["Resources/**/*"],
        exclude_directories = 0,
    )},

I left a comment on the rules_ios repo asking why we need this and how it works here.

The problem is not that files exist, it's that if you have both dir and dir/file in the inputs copy the dir first and then when we get to the file it's already there.

Also left the same comment in rules-ios PR.

In cocoapods-bazel, this line explicitly set exclude_directories = 0. This is where we have dirs as input. I will check if we can set this to 1 without losing any bundle file.

[ob]

In cocoapods-bazel, this line explicitly set exclude_directories = 0. This is where we have dirs as input. I will check if we can set this to 1 without losing any bundle file.

That's fine, but there might still be a bug in rules_apple where if exclude_directories is set to false it does the wrong thing and tries to copy over existing files. Although the fix in this PR will then perform poorly.

[ob]

I suggest we rename the PR to "if resource_bundles has exclude_directories = false, clonefile fails" or something like that.

[taowang487]

Thanks, @ob, changed as suggested, the new name makes more sense. Agreed with what you said, I think we might want to change this behavior in rules-apple. Other tools like XcodeGen behaves differently, which requires both foo/ and foo/bar as resources paths.

[brentleyjones]

Instead can we mkdir for the directories, and leave clonefile for files alone?

[taowang487]

@brentleyjones sounds good to me. you mean changing the behavior here right?

Shall I modify this PR to do this? Also this seems to be a breaking change.

[brentleyjones]

Hmm. Maybe not.

I'm going to double down on that I don't think it's valid for foo/ and foo/bar to both specified. The bundletool expects that if it sees a directory that it will copy everything under it, and that it won't later get asked to copy a file under that directory. rules_ios and others happened to work because the bundler was coping files twice (🙀). They should be fixed to only send directories when they want the full directory copied, and if they do that, to not send along files under those directories as well.

[taowang487]

Hmm. Maybe not.

I'm going to double down on that I don't think it's valid for foo/ and foo/bar to both specified. The bundletool expects that if it sees a directory that it will copy everything under it, and that it won't later get asked to copy a file under that directory. rules_ios and others happened to work because the bundler was coping files twice (🙀). They should be fixed to only send directories when they want the full directory copied, and if they do that, to not send along files under those directories as well.

@brentleyjones oh, I don't know this is an expected behavior. Can you please point me to the doc for this?

[keith]

Can you please point me to the doc for this?

There aren't any docs for this tool, it's private API so really rules_ios shouldn't use it at all and it could having breaking changes whenever (and it has in the past). In general I think the number of people who would read the docs on internal tools like this is so low that unfortunately it's not worth the time to document it and keep that up to date over time.

I'd also be interested to hear if fixing the double copies improves performance! In the past for our app bundletool was a major bottleneck so I imagine that could impact things if you have a lot of files.

[taowang487]

There aren't any docs for this tool, it's private API so really rules_ios shouldn't use it at all and it could having breaking changes whenever (and it has in the past).

I see, thanks for the explanation. I don't think rules-ios is accessing internal APIs. it only gathered all the resources and then pass to the bundle tool: https://github.com/bazel-ios/rules_ios/blob/master/rules/precompiled_apple_resource_bundle.bzl#L159-L185

[keith]

I mean bundletool itself is the private API. Also anything in internal packages like these imports https://github.com/bazel-ios/rules_ios/blob/56dfa68d77cf77fd12ba6f5f8332e3913f5ab8c9/rules/precompiled_apple_resource_bundle.bzl?rgh-link-date=2022-04-01T21%3A52%3A08Z#L11-L17 (buildifier warns about this, but maybe that's not enabled on rules_ios CI?)

[taowang487]

gotcha, thanks for the clarification. I thought these are some APIs provided by rules-apple.

[sanju-naik]

@larrytaowang I am also facing the exact same issue with GoogleMaps pod and to me, this happens only when I set --define=apple.experimental.tree_artifact_outputs=1 for now I have disabled this flag.

Since this PR is still open what was the solution you used?

[taowang487]

@larrytaowang I am also facing the exact same issue with GoogleMaps pod and to me, this happens only when I set --define=apple.experimental.tree_artifact_outputs=1 for now I have disabled this flag.

Since this PR is still open what was the solution you used?

@sanju-naik No per the discussion in this PR we don't want to change the behavior in rules-apple. We made some changes in our Build file for resource_bundles to get around this issue. But good to know that disabling --define=apple.experimental.tree_artifact_outputs=1 can also help! We are also using this flag.