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

Add enforceExplicitDependendencies generation option #5698

Merged
merged 15 commits into from
Jan 2, 2024

Conversation

fortmarek
Copy link
Member

@fortmarek fortmarek commented Dec 20, 2023

Short description 📝

The description of the problem

I prepared a fixture ios_app_with_implicit_dependencies that has the following graph:
image

What we're trying to enforce is that the relation between FrameworkA and FrameworkB is explicit. This is currently not the case due to all products being built into the same directory. Let's say we build FrameworkB first. The Build/Products directory will look like this:

  • Build/Products
    • FrameworkB.framework

Afterwards, when we build FrameworkA.framework, Xcode uses the combination of BUILT_PRODUCTS_DIR and FRAMEWORK_SEARCH_PATHS to find a particular framework.

Currently, FRAMEWORK_SEARCH_PATHS is empty but BUILT_PRODUCTS_DIR points directly to Build/Products. Since we built FrameworkB separately before building FrameworkA, FrameworkA successfully finds the FrameworkB.framework artifact in Build/Products and the build succeeds – even when there's no explicit relation between FrameworkA and FrameworkB.

Proposed solution

Soo, how do we propose to enforce describing the relation between FrameworkA and FrameworkB explicitly? By abandoning the Xcode's default behavior of dumping all built products into a single directory in Build/Products. Instead, we build frameworks into scoped directories.

To define where built artifacts are saved into, we can use the TARGET_BUILD_DIR setting. The default value for this setting is $(CONFIGURATION_BUILD_DIR)$(TARGET_BUILD_SUBPATH). To scope the artifacts, we will add an additional subpath equal to the product name: $(CONFIGURATION_BUILD_DIR)$(TARGET_BUILD_SUBPATH)/$(PRODUCT_NAME).

This is how the Build/Products directory looks like after building FrameworkB now:

  • Build/Products
    • FrameworkB
      • FrameworkB.framework

As described previously, Xcode tries to find available frameworks and build artifacts with the combination of BUILT_PRODUCTS_DIR and FRAMEWORK_SEARCH_PATHS. We also need to update these two settings, so that we can lookup only explicitly defined dependencies.

BUILT_PRODUCTS_DIR is updated to the same value is TARGET_BUILD_DIR: $(CONFIGURATION_BUILD_DIR)$(TARGET_BUILD_SUBPATH)/$(PRODUCT_NAME). That way, we ignore the root Build/Products for lookup of build artifacts. How will now FrameworkC successfully find FrameworkB – as it should, since the dependency is explicit?

For each explicit dependency, we add an entry in FRAMEWORK_SEARCH_PATHS, for FrameworkC that means $(CONFIGURATION_BUILD_DIR)$(TARGET_BUILD_SUBPATH)/FrameworkB.

To recap, these will be the build settings for individual modules of the graph:

  • FrameworkB
    • TARGET_BUILD_DIR: Build/Products/FrameworkB
    • BUILT_PRODUCTS_DIR:Build/Products/FrameworkB
    • FRAMEWORK_SEARCH_PATHS: Empty
  • FrameworkA
    • TARGET_BUILD_DIR: Build/Products/FrameworkA
    • BUILT_PRODUCTS_DIR:Build/Products/FrameworkA
    • FRAMEWORK_SEARCH_PATHS: Empty (FrameworkB won't be found)
  • FrameworkC
    • TARGET_BUILD_DIR: Build/Products/FrameworkC
    • BUILT_PRODUCTS_DIR:Build/Products/FrameworkC
    • FRAMEWORK_SEARCH_PATHS:Build/Products/FrameworkB (FrameworkB will be found)

Ensuring .app and other end products still work

We do these adjustments only for a couple of products:

  • dynamicLibrary, staticLibrary, framework, staticFramework, bundle

I did try doing the same adjustments for all products but this turned out be slightly more involved. For example, App can't rely on FRAMEWORK_SEARCH_PATHS only, but it actually needs all the built products (.framework, etc.) in the single directory specified by BUILT_PRODUCTS_DIR. We would need to copy or symlink all the individual directories of its transitive dependencies. While possible, this has the following drawbacks:

  • The Copy built products could have overlapping input and output paths if you had a scheme with two apps, leading to errors
  • Just copying those products is not enough for some scenarios – such as when running the host app

I fully expect most missing links to be between individual frameworks, not the final App target. That's why I decided for the following solution for end products like .app:

Each module with a scoped TARGET_BUILD_DIR also symlinks its built products into the default Build/Products directory. App keeps using this default directory, so it will find all necessary products – but other frameworks will still use only its scoped directory with framework search paths.

This does mean each framework will have an extra post build script – but we have an explicit set of input and output paths and we only override the symlink when the symlink doesn't exist. That should ensure the incremental compilation is not impacted. Additionally, these copied built products are only used by end products – other frameworks depend on FRAMEWORK_SEARCH_PATHS instead.

External frameworks

While ideally the graph for external frameworks would be valid, there's not much developers can do if explicit links are missing. I believe it's better to scope all external frameworks into a single Build/Products/External directory that internal targets use for lookup, so the explicitness is not required for them.

How to test the changes locally 🧐

Run tuist generate on one of the fixtures or your own project and ensure that it works as expected (you can build it, run it, etc.)

Contributor checklist ✅

  • The code has been linted using run make workspace/lint-fix
  • The change is tested via unit testing or acceptance testing, or both
  • The title of the PR is formulated in a way that is usable as a changelog entry
  • In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist ✅

  • The code architecture and patterns are consistent with the rest of the codebase
  • Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

Comment on lines 40 to 42
additionalSettings["TARGET_BUILD_DIR"] = "$(CONFIGURATION_BUILD_DIR)$(TARGET_BUILD_SUBPATH)/$(PRODUCT_NAME)"

additionalSettings["BUILT_PRODUCTS_DIR"] = "$(CONFIGURATION_BUILD_DIR)$(TARGET_BUILD_SUBPATH)/$(PRODUCT_NAME)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are both needed?

Comment on lines 50 to 54
TargetScript(
name: "Copy Build Products",
order: .pre,
script: .embedded(copyProductsScript)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it possible to tell Xcode to build a product directly into a different directory? I thought that'd be controllable via build settings. If not, wouldn't we still have the same issue since we are only creating a symbolic link? I think I'm missing something in this logic.

Comment on lines 11 to 24
let movedProductNames = target.dependencies.compactMap {
switch $0 {
case
let .target(name: name, condition: _),
let .project(target: name, path: _, condition: _):
return name
case .library, .framework, .package, .sdk, .xcframework, .xctest:
return nil
}
}

let frameworkSearchPaths = movedProductNames.map {
"$(CONFIGURATION_BUILD_DIR)$(TARGET_BUILD_SUBPATH)/\($0)"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about turning this one into a graph mapper and having something along the lines of:

let directDependencyTargets = GraphTraverser(graph: graph).directTargetDependencies(name: target.name, project: project)
let frameworkSearchPaths = directDependencyTargets.map(\.target.buildProductDirectoryBuildSetting)

Copy link
Collaborator

@kwridan kwridan left a comment

Choose a reason for hiding this comment

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

Thanks @fortmarek

Do you mind sharing more details on the issue and what the mapper is trying to solve?

From past discussions I recall we flagged an issue where if a target (e.g. FrameworkA) was using another target (e.g. FrameworkB) but didn't explicitly declare a dependency on said target in its manifest can result in a build failure if you were to build FrameworkA in isolation. Is this what the issue being addressed here?

For that use case I don't think we should try to do anything with mappers to work around the issue, we discussed a possibility to either add a lint rule or plugin to perform the linting to identify and surface these issues for developers to address (e.g. by scanning sources and identifying any imported frameworks that aren't declared in the manifest).

@@ -79,6 +79,8 @@ public final class ProjectMapperFactory: ProjectMapperFactorying {

// Signing
mappers.append(SigningMapper())

mappers.append(TargetProjectMapper(mapper: ExplicitDependencyTargetMapper()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this apply to all generated projects by tuist by default or in some cases like caching only? 🤔

I'd be very cautious about introducing an additional script phase to all projects. Is it trying to solve a problem every project has or a specific edge case? would be helpful to understand the issue better.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be hidden behind a feature flag or a config option, initially. And based on the feedback we receive, we can think of rolling it out further or adding this mapper by default for some commands like tuist cache warm.

@fortmarek fortmarek changed the title Add mapper to enforce explicit dependencies Add enforceExplicitDependendencies generation option Dec 29, 2023
@fortmarek fortmarek marked this pull request as ready for review December 29, 2023 18:42
@fortmarek fortmarek added the changelog:added PR will be listed in the Added section of CHANGELOG label Jan 2, 2024
@fortmarek fortmarek merged commit b14eba5 into main Jan 2, 2024
8 of 9 checks passed
@fortmarek fortmarek deleted the mapper/explicit-dependencies branch January 2, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:added PR will be listed in the Added section of CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants