-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Remove slightly dangerous optimization #4317
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
Conversation
@swift-ci please smoke test |
Looks like there are various tests asserting the current behavior, so converting to draft for now until I get to fixing them. |
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.
Looks good to me in principle.
6c49273
to
3c2ed6a
Compare
@swift-ci please smoke test |
@@ -589,7 +589,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests { | |||
|
|||
XCTAssertNoDiagnostics(observability.diagnostics) | |||
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1), [manifestPath]) | |||
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1), expectCached ? [] : [manifestPath]) | |||
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, expectCached ? 0 : 1) |
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.
Switching to count
here since the file name doesn't seem relevant.
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.
looks good, a couple of small suggestions
@neonichu do we want to take this one? |
Yah, definitely, I need to come back to it and implement your suggestions. |
3c2ed6a
to
14ceb3e
Compare
@swift-ci please smoke test |
@swift-ci please smoke test macOS |
@swift-ci please smoke test macOS |
d46bb5a
to
eabbbdc
Compare
@swift-ci please smoke test |
eabbbdc
to
a2f1317
Compare
Fixed this so that we pass the original path to the compiler, this makes the VFS actually be used. Still seeing one test failure in |
Looks like this is maybe breaking |
Ah no, the problem is that the test was only working due to the bug where we would present the temporary file as the "real" manifest. Prior to my change:
After my change:
Since the manifest we're being told to load is |
@swift-ci please smoke test |
a2f1317
to
198a654
Compare
Forgot to actually push my changes before re-running 🤦 |
@swift-ci please smoke test |
if localFileSystem.isFile(manifestPath) { | ||
try withTemporaryDirectory { tempDir, cleanupTempDir in | ||
let manifestTempFilePath = tempDir.appending(component: "manifest.swift") | ||
try localFileSystem.writeFileContents(manifestTempFilePath, bytes: ByteString(manifestContents)) |
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 change looks good in that it gets rid of the special-casing. Just so I understand, it does change so that now we always write the temporary file, whether or not we already have an original source file?
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.
That's right, for two reasons:
- this could lead to reading from the wrong file, because we were always reading from the local FS, no matter which one we were actually using to get the contents of the manifest (this originally sparked this PR)
- I'd like us to gain the ability to add to the manifest (e.g. by adding imports), this will allow things like making our non-implementation-only import of Foundation conditional on older tools-versions
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.
Absolutely, I was thinking along exactly the same lines for bullet two below. Also for automatically getting struct definitions from things like plugins. As long as the VFS option is solid in terms of the diagnostics (which the tests should show), then I think this is great.
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.
Yah, diagnostics look solid and also other context, like the delegate methods etc. We're now always passing down the original path to the loading machinery and the only thing that knows about the "real" path is the VFS.
I think I just forgot to add that to the CMake config. |
If there happens to be a file at the location passed into the `ManifestLoader, we were using that to compile the manifest, instead of the actual contents. This is dangerous, because the public API of `ManifestLoader` accepts a custom filesystem parameter that is not taken into account at all here, so the file being used might not be the right one, it just has the same path on the local filesystem. We could have done some more work to preserve the optimization in the case where the used filesystem is the local one and the correct file is being used, but that doesn't seem worth it since the vast majority of loads are being done for remote dependencies (this we may load multiple times as we are resolving) which are never using the local filesystem.
198a654
to
bd6e3fd
Compare
@swift-ci please smoke test |
If there happens to be a file at the location passed into the `ManifestLoader, we were using that to compile the manifest, instead of the actual contents. This is dangerous, because the public API of `ManifestLoader` accepts a custom filesystem parameter that is not taken into account at all here, so the file being used might not be the right one, it just has the same path on the local filesystem. We could have done some more work to preserve the optimization in the case where the used filesystem is the local one and the correct file is being used, but that doesn't seem worth it since the vast majority of loads are being done for remote dependencies (this we may load multiple times as we are resolving) which are never using the local filesystem.
If there happens to be a file at the location passed into the
ManifestLoader
, we were using that to compile the manifest, instead of the actual contents. This is dangerous, because the public API ofManifestLoader
accepts a custom filesystem parameter that is not taken into account at all here, so the file being used might not be the right one, it just has the same path on the local filesystem.We could have done some more work to preserve the optimization in the case where the used filesystem is the local one and the correct file is being used, but that doesn't seem worth it since the vast majority of loads are being done for remote dependencies (this we may load multiple times as we are resolving) which are never using the local filesystem.