-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
🛑 Prevent overwriting identical workspace data #607
Conversation
Codecov Report
@@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 84.28% 84.20% -0.09%
==========================================
Files 154 154
Lines 8688 8700 +12
==========================================
+ Hits 7323 7326 +3
- Misses 1365 1374 +9
Continue to review full report at Codecov.
|
@ferologics to resolve the error, could you try running |
f69787e
to
0774ce3
Compare
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.
Thanks @ferologics! This has been one annoying quirk of regenerating projects that would be great to solve.
@yonaskolb would this have any negative side effects for XcodeGen?
From my testing, I noticed if you have a workspace with several projects open, Xcode seems to tolerate regenerating the individual projects, however if you regenerate the entire workspace (including the .xcworkspace
) that's when we start hitting the issue of needing to close and re-open Xcode.
Prototyping locally something like this may do the trick:
- extracted a
rawContents()
method inXCWorkspaceData
which can be used by its write method
// XCWorkspaceData.swift
func rawContents() -> String {
let document = AEXMLDocument()
let workspace = document.addChild(name: "Workspace", value: nil, attributes: ["version": "1.0"])
_ = children
.map { $0.xmlElement() }
.map(workspace.addChild)
return document.xmlXcodeFormat
}
// MARK: - <Writable>
public func write(path: Path, override: Bool = true) throws {
let rawXml = rawContents()
if override, path.exists {
try path.delete()
}
try path.write(rawXml)
}
- Updated
XCWorkspace.write
to check the raw contents before deleting
// XCWorkspace.swift
public func write(path: Path, override: Bool = true) throws {
let dataPath = path + "contents.xcworkspacedata"
if override, dataPath.exists {
if let existingContent: String = try? dataPath.read(),
existingContent == data.rawContents() {
// Raw data matches what's on disk
// there's no need to overwrite the contents
// this mitigates Xcode forcing users to
// close and re-open projects/workspaces
// on regneration.
return
}
try dataPath.delete()
}
try dataPath.mkpath()
try data.write(path: dataPath)
}
Benchmarking reading the raw contents vs deserialisation they were roughly the same - not sure now which is better 🤷♂️
@@ -130,6 +130,7 @@ extension XcodeProj: Writable { | |||
/// - Parameter override: if workspace should be overridden. Default is true. | |||
/// If false will throw error if workspace already exists at the given path. | |||
public func writeWorkspace(path: Path, override: Bool = true) throws { | |||
guard let p = path.glob("*.xcworkspacedata").first, workspace.data != (try? XCWorkspaceData(path: p)) else { return } |
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.
There's a few caveats we should be mindful here:
- The
xcworkspacedata
serialisation is a responsibility of theXCWorkspace
class, a check of this nature can potentially go there, as that component knows the exact path and there wouldn't be a need to search / glob for it :) XCWorkspace
deletes and recreates the underlyingcontents.xcworkspacedata
file whenoverride
is set, which I believe may be the source of the issue - this is also the same when viewing a Workspace rather than a standalone Project
if override, dataPath.exists {
try dataPath.delete()
}
- We need to mindful of not reloading and deserialising content from disk to verify equality as that could potentially be expensive for large workspaces (needs benchmarking), we may be able to compare the raw data without deserialisation - From my quick test locally on a test fixture with 100 projects it didn't have as big of an impact as I feared it would <1%
Thanks for giving this a thorough look @kwridan 🙇 I agree with your observations about both the responsibility and the root cause. I adjusted the code according to your suggestion. |
After spending more time debugging this with a project I still got the occasional "The file “project.xcworkspace” has been modified by another application." when regenerating. A closer look at the Clean slate: <?xml version="1.0" encoding="UTF-8"?>
<Workspace
version = "1.0">
</Workspace> Existing: <?xml version="1.0" encoding="UTF-8"?>
<Workspace
version = "1.0">
<FileRef
location = "self:">
</FileRef>
</Workspace> The problematic |
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.
The problematic is added at some point in the lifetime of the project. I am not exactly sure what it's doing there, or what it's purpose is
This comes from XCWorkspaceDataElementLocationType
that has a case
self(String) // Single project workspace in xcodeproj directory
. I have not dived deeper into why this one is added as I don't really follow what's the issue with it - the generation should be deterministic, so what exact modification / command creates this change in the data? But I believe we are fine with the fact that this data changes as long as it happens only when the generated .xcworkspace
is different as well.
Co-authored-by: Marek Fořt <[email protected]>
I believe what you're witnessing is an internal Xcode behaviour: My current understanding of it based on past observations is as follows:
<FileRef
location = "self:">
</FileRef>
<?xml version="1.0" encoding="UTF-8"?>
<Workspace
version = "1.0">
<FileRef
location = "group:App/App.xcodeproj">
</FileRef>
<FileRef
location = "group:Modules/MyFramework.xcodeproj">
</FileRef>
</Workspace> As such we'd need to figure out which |
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.
Thanks for the updates
Partially resolves tuist/tuist#2809
for projects usingApp.xcodeproj
-- withoutApp.xcworkspace
at project root. I haven't investigated the issue beyond the scope of.xcodeproj
generation.Edit: The implementation has been updated to prevent overwriting identical workspace data (regardless of whether it's a project or a workspace)
Short description 📝
Prevent overwriting identical workspace data
Solution 📦
Check if previous workspace data is equal to newly generated workspace data.
Implementation 👩💻👨💻
XCWorkspace
that prevents overwriting of identical dataHow can I test this change @fortmarek? I ran into complications when trying with
1.40.0
tag viaswift build -c release --product tuist
and using that with Xcode12.4
version: