Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid generating a CompileAssetCatalog build phase #43

Merged
merged 2 commits into from
Apr 28, 2020

Conversation

amberdixon
Copy link
Collaborator

For xcasset sources, we do not want xcode to run actool. I'm not sure how to stub out the CompileAssetCatalog command, so it's easier to just load xcasset files into the xcodeproject, but not associate them with any build phase.

@amberdixon amberdixon requested a review from segiddins April 16, 2020 05:06
"group": paths.dirname(s.short_path),
"optional": True,
}
if ".xcassets" in s.path:
Copy link
Member

Choose a reason for hiding this comment

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

do we actually maybe want to filter out everything that would go into the PBXResourcesBuildPhase, instead of only asset catalogs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure how to figure out what all these possible things were, but I would have preferred to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Take a look at _bucketize in https://github.com/bazelbuild/rules_apple/blob/9c1e460ec08af6e36673d6c6f34c723b09ae9328/apple/internal/resources.bzl -- I think everything explicitly called out there (e.g. not "unprocessed") could be ignore.

Another approach would be to propagate 2 different fields in the provider -- one for explicit srcs that come from the srcs attribute and another with basically everything else. Everything not in srcs would get added without a build phase. That's probably the more elegant way of handling this, rather than hardcoding based on extensions

…plists, asset catalogs, etc.) Continue to load those files into the xcode project.
@amberdixon amberdixon force-pushed the amber/avoid-actool-invocations branch from 3e2292f to 9212352 Compare April 28, 2020 03:48
Copy link
Contributor

@gyfelton gyfelton left a comment

Choose a reason for hiding this comment

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

I consider this as a super important PR I am going to keep referring to because it bridges a concept well known (asset catalog) to the existing framework we have and it has tests that shows how each component link together. (a very important PR to study for any newbie)

@@ -31,7 +31,6 @@ def _xcodeproj_aspect_impl(target, ctx):
test_commandline_args = ()
if AppleBundleInfo in target:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to understand this if stmt because the code change applies to all blocks of this if stmts correct?
there are

  1. AppleBundleInfo in target
  2. ctx.rule.kind == "apple_framework_packaging"
  3. else
    I wonder each is for what cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In practice:

  1. if AppleBundleInfo in target is generally applied to targets that are unit test bundles or application bundles.
  2. When the target is packaged up framework (this target usually depends on the components of a framework, like libraries, resource bundles, plists.)
  3. Everything else. If the target is not performing a bundling/packaging operation, it often contains some kind of source files and dependencies with their source files.

@amberdixon amberdixon merged commit d4e2510 into master Apr 28, 2020
@justinseanmartin
Copy link
Collaborator

Sorry for the late review, but LGTM

@justinseanmartin justinseanmartin deleted the amber/avoid-actool-invocations branch April 29, 2020 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants