Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Commit

Permalink
Facilitate removing FS access from AndroidResource transformation
Browse files Browse the repository at this point in the history 
Summary:
Add an option to specify the inputs for `android_resource` as a map
instead of passing the top-level directory as a source path so that
we can avoid filesystem access during the target graph to action
graph transformation.

With this change, there are three possible ways resources and
assets can be treated:
  # They are specified as a PathSourcePath. This used to be the
    most common way. In this case the contents of the directory
    are still traversed during the transformation, but then the
    filtered files are placed in a symlink tree to ensure the
    filtered out files are not used.
  # They are specified as a BuildTargetSourcePath. This was not as
    common and the behavior in this case does not change. There is
    no FS access and the output of the rule is used in whole.
  # They are specified as a map from relative path to source path.
    This is a new option that behaves similarly to the first case,
    except no FS access is necessary.

Future plan is to migrate all users to cases 2 or 3 and then either
disable case 1 or remove the filesystem access from it which will
mean that no filtering will take case and rule keys might diverge
if ignored files are present.

Test Plan: * `buck test`.

Reviewed By: Coneko

fbshipit-source-id: 7af04c3
  • Loading branch information
@k21 @facebook-github-bot
k21 authored and facebook-github-bot committed Jan 26, 2017
1 parent 9688ec3 commit 3319cd6
Show file tree
Hide file tree
Showing 31 changed files with 732 additions and 223 deletions.
34 changes: 31 additions & 3 deletions docs/rule/android_resource.soy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ generated via <code>aapt --output-text-symbols</code>.
{param name: 'res' /}
{param default : 'None' /}
{param desc}
Path to a directory containing Android resources.
A dictionary mapping relative resource paths to either
the resource files or the build targets that generate them.
The <a href="{ROOT}function/subdir_glob.html"><code>subdir_glob()</code></a> function
can be used to generate dictionaries based on a directory structure of files checked
into the repository. Alternatively, this can be a path to a directory containing
Android resources, although this option is deprecated and might be removed in the future.
{/param}
{/call}

Expand All @@ -52,7 +57,30 @@ generated via <code>aapt --output-text-symbols</code>.
{param name: 'assets' /}
{param default : 'None' /}
{param desc}
Path to a directory containing Android assets.
A dictionary mapping relative asset paths to either
the asset files or the build targets that generate them.
The <a href="{ROOT}function/subdir_glob.html"><code>subdir_glob()</code></a> function
can be used to generate dictionaries based on a directory structure of files checked
into the repository. Alternatively, this can be a path to a directory containing
Android assets, although this option is deprecated and might be removed in the future.
{/param}
{/call}

{call buck.arg}
{param name: 'project_res' /}
{param default : 'None' /}
{param desc}
A directory containing resources to be used for project generation.
If not provided, defaults to whatever the build uses.
{/param}
{/call}

{call buck.arg}
{param name: 'project_assets' /}
{param default : 'None' /}
{param desc}
A directory containing assets to be used for project generation.
If not provided, defaults to whatever the build uses.
{/param}
{/call}

Expand Down Expand Up @@ -80,7 +108,7 @@ such simple rules are often named <code>res</code>:
{literal}<pre class="prettyprint lang-py">
android_resource(
name = 'res',
res = 'res',
res = subdir_glob([('res', '**')]),
package = 'com.example',
)
</pre>{/literal}
Expand Down
7 changes: 3 additions & 4 deletions src/com/facebook/buck/android/AndroidAarDescription.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableSortedSet;

import java.nio.file.Path;
Expand Down Expand Up @@ -176,12 +177,10 @@ public <A extends Arg> BuildRule createBuildRule(
.addAll(originalBuildRuleParams.getDeclaredDeps().get())
.build(),
new BuildTargetSourcePath(assembleResourceDirectories.getBuildTarget()),
/* resSrcs */ ImmutableSortedSet.of(),
Optional.empty(),
/* resSrcs */ ImmutableSortedMap.of(),
/* rDotJavaPackage */ null,
new BuildTargetSourcePath(assembleAssetsDirectories.getBuildTarget()),
/* assetsSrcs */ ImmutableSortedSet.of(),
Optional.empty(),
/* assetsSrcs */ ImmutableSortedMap.of(),
new BuildTargetSourcePath(manifest.getBuildTarget()),
/* hasWhitelistedStrings */ false);
aarExtraDepsBuilder.add(resolver.addToIndex(androidResource));
Expand Down
51 changes: 16 additions & 35 deletions src/com/facebook/buck/android/AndroidResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,18 +42,19 @@
import com.facebook.buck.step.fs.WriteFileStep;
import com.facebook.buck.util.HumanReadableException;
import com.facebook.buck.util.MoreCollectors;
import com.facebook.buck.util.MoreMaps;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Ordering;

import java.nio.file.Path;
import java.util.Optional;
import java.util.concurrent.atomic.AtomicReference;

import javax.annotation.Nullable;
Expand Down Expand Up @@ -85,29 +86,21 @@ public class AndroidResource extends AbstractBuildRule
@VisibleForTesting
static final String METADATA_KEY_FOR_R_DOT_JAVA_PACKAGE = "METADATA_KEY_FOR_R_DOT_JAVA_PACKAGE";

@AddToRuleKey
@Nullable
private final SourcePath res;

/** contents of {@code res} under version control (i.e., not generated by another rule). */
@SuppressWarnings("PMD.UnusedPrivateField")
@AddToRuleKey
private final ImmutableSortedSet<? extends SourcePath> resSrcs;
private final ImmutableSortedMap<String, SourcePath> resSrcs;

@SuppressWarnings("PMD.UnusedPrivateField")
@AddToRuleKey
private final Optional<SourcePath> additionalResKey;

@Nullable
private final SourcePath assets;

/** contents of {@code assets} under version control (i.e., not generated by another rule). */
@SuppressWarnings("PMD.UnusedPrivateField")
@AddToRuleKey
private final ImmutableSortedSet<? extends SourcePath> assetsSrcs;

@SuppressWarnings("PMD.UnusedPrivateField")
@AddToRuleKey
private final Optional<SourcePath> additionalAssetsKey;
private final ImmutableSortedMap<String, SourcePath> assetsSrcs;

private final Path pathToTextSymbolsDir;
private final Path pathToTextSymbolsFile;
Expand Down Expand Up @@ -159,13 +152,11 @@ public AndroidResource(
BuildRuleParams buildRuleParams,
SourcePathRuleFinder ruleFinder,
final ImmutableSortedSet<BuildRule> deps,
@Nullable final SourcePath res,
ImmutableSortedSet<? extends SourcePath> resSrcs,
Optional<SourcePath> additionalResKey,
@Nullable SourcePath res,
ImmutableSortedMap<Path, SourcePath> resSrcs,
@Nullable String rDotJavaPackageArgument,
@Nullable SourcePath assets,
ImmutableSortedSet<? extends SourcePath> assetsSrcs,
Optional<SourcePath> additionalAssetsKey,
ImmutableSortedMap<Path, SourcePath> assetsSrcs,
@Nullable SourcePath manifestFile,
Supplier<ImmutableSortedSet<? extends SourcePath>> symbolFilesFromDeps,
boolean hasWhitelistedStrings,
Expand All @@ -182,11 +173,9 @@ public AndroidResource(
}

this.res = res;
this.resSrcs = resSrcs;
this.additionalResKey = additionalResKey;
this.resSrcs = MoreMaps.transformKeysAndSort(resSrcs, Path::toString);
this.assets = assets;
this.assetsSrcs = assetsSrcs;
this.additionalAssetsKey = additionalAssetsKey;
this.assetsSrcs = MoreMaps.transformKeysAndSort(assetsSrcs, Path::toString);
this.manifestFile = manifestFile;
this.symbolsOfDeps = symbolFilesFromDeps;
this.hasWhitelistedStrings = hasWhitelistedStrings;
Expand Down Expand Up @@ -222,13 +211,11 @@ public AndroidResource(
final BuildRuleParams buildRuleParams,
SourcePathRuleFinder ruleFinder,
final ImmutableSortedSet<BuildRule> deps,
@Nullable final SourcePath res,
ImmutableSortedSet<? extends SourcePath> resSrcs,
Optional<SourcePath> additionalResKey,
@Nullable SourcePath res,
ImmutableSortedMap<Path, SourcePath> resSrcs,
@Nullable String rDotJavaPackageArgument,
@Nullable SourcePath assets,
ImmutableSortedSet<? extends SourcePath> assetsSrcs,
Optional<SourcePath> additionalAssetsKey,
ImmutableSortedMap<Path, SourcePath> assetsSrcs,
@Nullable SourcePath manifestFile,
boolean hasWhitelistedStrings) {
this(
Expand All @@ -237,11 +224,9 @@ public AndroidResource(
deps,
res,
resSrcs,
additionalResKey,
rDotJavaPackageArgument,
assets,
assetsSrcs,
additionalAssetsKey,
manifestFile,
hasWhitelistedStrings,
/* resourceUnion */ false,
Expand All @@ -252,13 +237,11 @@ public AndroidResource(
final BuildRuleParams buildRuleParams,
SourcePathRuleFinder ruleFinder,
final ImmutableSortedSet<BuildRule> deps,
@Nullable final SourcePath res,
ImmutableSortedSet<? extends SourcePath> resSrcs,
Optional<SourcePath> additionalResKey,
@Nullable SourcePath res,
ImmutableSortedMap<Path, SourcePath> resSrcs,
@Nullable String rDotJavaPackageArgument,
@Nullable SourcePath assets,
ImmutableSortedSet<? extends SourcePath> assetsSrcs,
Optional<SourcePath> additionalAssetsKey,
ImmutableSortedMap<Path, SourcePath> assetsSrcs,
@Nullable SourcePath manifestFile,
boolean hasWhitelistedStrings,
boolean resourceUnion,
Expand All @@ -269,11 +252,9 @@ public AndroidResource(
deps,
res,
resSrcs,
additionalResKey,
rDotJavaPackageArgument,
assets,
assetsSrcs,
additionalAssetsKey,
manifestFile,
() -> FluentIterable.from(buildRuleParams.getDeps())
.filter(HasAndroidResourceDeps.class)
Expand Down
Loading

8 comments on commit 3319cd6

@kageiit
Copy link
Contributor

@kageiit kageiit commented on 3319cd6 Jan 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that we can finally specify multiple resource/asset dirs per android_resource rule?

because currently in the gradle world with flavors we can have src/main/res and src/flavor/res.

We generate two android_resource rules and add them as a dependency to the android_library. But its a problem because in gradle they are treated as part of the same android_resource equivalent so resources in main can be accessed in flavor and vice versa. This cannot be possible in buck because it would create a cyclic dependency. It would be great with this change that we can just have multiple directories containing resources be consumed as part of the same android_resource rule

@kageiit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @k21 @dreiss

@dreiss
Copy link
Member

@dreiss dreiss commented on 3319cd6 Jan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. The purpose of this change was actually just to make things faster while preserving our existing semantics, so it might not work exactly as you expect. Give it a shot and let us know how it goes.

@kageiit
Copy link
Contributor

@kageiit kageiit commented on 3319cd6 Jan 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok this sorta works.

The android_resource rule looks something like

android_resource(
	name = 'res_flavor',
	package = 'com.uber.okbuck.example.common',
	res = subdir_glob([
		('src/main/res', '**'),
		('src/flavor/res', '**'),
	]),

The only restriction seems to be that there cannot be a file with the same name inside both directories i.e there cannot be a values/strings.xml in src/flavor/res if that path also exists under src/main/res. However if the file is renamed to something like src/flavor/res/values/flavor_strings.xml, the resource rule builds fine. This seems to be a restriction imposed by the implementation of subdir_glob as it seems that it was originally only intended for C/C++ header maps (https://buckbuild.com/function/subdir_glob.html)

BUILD FAILED: Parse error for build file /Users/kage/src/okbuck/libraries/common/BUCK:
AssertionError: Conflicting header files in header search paths. "values/strings.xml" maps to both "src/main/res/values/strings.xml" and "src/paidDebug/res/values/strings.xml".
Call stack:
  File "/Users/kage/.gradle/caches/okbuck/buck/build/classes/buck_parser/buck.py", line 1221, in process_with_diagnostics
    diagnostics=diagnostics)
  File "/Users/kage/.gradle/caches/okbuck/buck/build/classes/buck_parser/buck.py", line 1111, in process
    implicit_includes=self._implicit_includes)
  File "/Users/kage/.gradle/caches/okbuck/buck/build/classes/buck_parser/buck.py", line 1044, in _process_build_file
    implicit_includes=implicit_includes)
  File "/Users/kage/.gradle/caches/okbuck/buck/build/classes/buck_parser/buck.py", line 966, in _process
    exec(code, module.__dict__)
  File "/Users/kage/src/okbuck/libraries/common/BUCK", line 158
    ('src/paidDebug/res', '**'),
  File "/Users/kage/.gradle/caches/okbuck/buck/build/classes/buck_parser/buck.py", line 718, in _subdir_glob
    allow_safe_import=self._import_whitelist_manager.allow_unsafe_import)
  File "/Users/kage/.gradle/caches/okbuck/buck/build/classes/buck_parser/buck.py", line 449, in subdir_glob
    return merge_maps(*results)
  File "/Users/kage/.gradle/caches/okbuck/buck/build/classes/buck_parser/buck.py", line 394, in merge_maps
    % (key, result[key], header_map[key])

Maybe we can add an option to subdir_glob to lift this restriction and then we can finally have the ability to specify multiple res/assets directories per an android_resource rule

@kageiit
Copy link
Contributor

@kageiit kageiit commented on 3319cd6 Jan 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a quick proof of concept, I made the following changes to src/com/facebook/buck/json/buck_parser/buck.py

def merge_maps(*header_maps):
    result = {}
    for header_map in header_maps:
        for key in header_map:
            if key in result and result[key] != header_map[key]:
                parts = key.split('/')
                prefix = parts[1]
                new_key = '/'.join(parts[:-1]) + '/' + prefix + '_' + parts[-1]
                # assert False, 'Conflicting header files in header search paths. ' + \
                #               '"%s" maps to both "%s" and "%s".' \
                #               % (key, result[key], header_map[key])
                result[new_key] = header_map[key]

            result[key] = header_map[key]

    return result

This basically made the map key not be the same but prefixed the file name with the source folder flavor name automatically. This is of course not a final solution but just showing the possibility that we can add an option to subdir_glob to use some sane scheme for what to do about duplicate keys. If we can maybe add a numbered prefix before the duplicate key for example, that would achieve the same result without affecting the cache keys of the rule by a great deal

@kageiit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k21 @dreiss would appreciate some input on the above suggestions/changes. I can open a follow up issue for this

@Coneko
Copy link

@Coneko Coneko commented on 3319cd6 Jan 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to change the behaviour of subdir_glob, it's just a builtin function available in build files, there's nothing special about it.

You can easily define your own globbing function that handles duplicate keys in a defs file you include and use that instead.

@kageiit
Copy link
Contributor

@kageiit kageiit commented on 3319cd6 Jan 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Coneko thanks for the suggestion. I have added support for the use case in uber/okbuck#361

Please sign in to comment.