-
Notifications
You must be signed in to change notification settings - Fork 873
Fix synced folder issues: configFiles duplicate, group insertion, and directory-level membershipExceptions #1607
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
391bd18
f31985a
494fc9e
cf85270
d59b1c7
574693f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -206,7 +206,13 @@ class SourceGenerator { | |
| let createIntermediateGroups = project.options.createIntermediateGroups | ||
|
|
||
| let parentPath = path.parent() | ||
|
|
||
| guard !isInsideSyncedFolder(path: path) else { | ||
| return getFileReference(path: path, inPath: project.basePath, sourceTree: .sourceRoot) | ||
| } | ||
|
|
||
| let fileReference = getFileReference(path: path, inPath: parentPath) | ||
|
|
||
| let parentGroup = getGroup( | ||
| path: parentPath, | ||
| mergingChildren: [fileReference], | ||
|
|
@@ -283,6 +289,19 @@ class SourceGenerator { | |
| } | ||
| } | ||
|
|
||
| /// Whether the given path falls inside a target source configured as a synced folder. | ||
| /// Checks the project spec directly because configFiles are resolved before target sources | ||
| /// populate `syncedGroupsByPath`. | ||
| private func isInsideSyncedFolder(path: Path) -> Bool { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm concerned about the performance implications of such a loop
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point. In practice it's pretty small I think because it is only called from two cold paths ( But I can easily cache it with a lazy, it could look like this: private lazy var syncedFolderPrefixes: Set<String> = {
var prefixes = Set<String>()
for target in project.targets {
for source in target.sources {
let type = source.type ?? (project.options.defaultSourceDirectoryType ?? .group)
if type == .syncedFolder {
prefixes.insert(source.path + "/")
}
}
}
return prefixes
}()
private func isInsideSyncedFolder(path: Path) -> Bool {
let relativePath = (try? path.relativePath(from: basePath)) ?? path
return syncedFolderPrefixes.contains { relativePath.string.hasPrefix($0) }
}
´´´
Want me to go with that? |
||
| let relativePath = (try? path.relativePath(from: project.basePath)) ?? path | ||
| return project.targets.contains { target in | ||
| target.sources.contains { source in | ||
| let type = source.type ?? (project.options.defaultSourceDirectoryType ?? .group) | ||
| return type == .syncedFolder && relativePath.string.hasPrefix(source.path + "/") | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// returns a default build phase for a given path. This is based off the filename | ||
| private func getDefaultBuildPhase(for path: Path, targetType: PBXProductType) -> BuildPhaseSpec? { | ||
| if let buildPhase = getFileType(path: path)?.buildPhase { | ||
|
|
@@ -356,7 +375,7 @@ class SourceGenerator { | |
| groupReference = addObject(group) | ||
| groupsByPath[path] = groupReference | ||
|
|
||
| if isTopLevelGroup { | ||
| if isTopLevelGroup && !isInsideSyncedFolder(path: path) { | ||
| rootGroups.insert(groupReference) | ||
| } | ||
| } | ||
|
|
@@ -402,6 +421,8 @@ class SourceGenerator { | |
| if child.isDirectory && !Xcode.isDirectoryFileWrapper(path: child) { | ||
| findExceptions(in: child) | ||
| } | ||
| } else if child.isDirectory && !Xcode.isDirectoryFileWrapper(path: child) { | ||
| findExceptions(in: child) | ||
| } else { | ||
| exceptions.insert(child) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have a new
basePathgetter that may be a better value, as xcodegen can be run in a different directory than the project base path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I did't see it.
I just changed with
basePath, it's way more cleaner