Skip to content

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

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

neonichu
Copy link
Contributor

@neonichu neonichu commented Apr 22, 2022

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.

@neonichu
Copy link
Contributor Author

@swift-ci please smoke test

@neonichu neonichu self-assigned this Apr 22, 2022
@neonichu neonichu marked this pull request as draft April 22, 2022 04:40
@neonichu
Copy link
Contributor Author

Looks like there are various tests asserting the current behavior, so converting to draft for now until I get to fixing them.

Copy link
Contributor

@abertelrud abertelrud left a 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.

@tomerd tomerd added the WIP Work in progress label Jul 12, 2022
@neonichu neonichu force-pushed the remove-manifest-loading-optimization branch from 6c49273 to 3c2ed6a Compare September 2, 2022 22:07
@neonichu
Copy link
Contributor Author

neonichu commented Sep 2, 2022

@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)
Copy link
Contributor Author

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.

@neonichu neonichu marked this pull request as ready for review September 2, 2022 22:18
@neonichu neonichu removed the WIP Work in progress label Sep 2, 2022
Copy link
Contributor

@tomerd tomerd left a 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

@tomerd
Copy link
Contributor

tomerd commented Oct 28, 2022

@neonichu do we want to take this one?

@neonichu
Copy link
Contributor Author

Yah, definitely, I need to come back to it and implement your suggestions.

@neonichu neonichu force-pushed the remove-manifest-loading-optimization branch from 3c2ed6a to 14ceb3e Compare November 1, 2022 23:23
@neonichu
Copy link
Contributor Author

neonichu commented Nov 1, 2022

@swift-ci please smoke test

@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

@swift-ci please smoke test macOS

@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

@swift-ci please smoke test macOS

@neonichu neonichu force-pushed the remove-manifest-loading-optimization branch from d46bb5a to eabbbdc Compare November 2, 2022 06:11
@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

@swift-ci please smoke test

@neonichu neonichu force-pushed the remove-manifest-loading-optimization branch from eabbbdc to a2f1317 Compare November 2, 2022 16:40
@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

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 testPackageContextDirectory.

@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

Looks like this is maybe breaking manifest.displayName as it looks empty during testPackageContextDirectory with my changes.

@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

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:

(lldb) po packageDirectory
"/var/folders/jc/zkdxhl4j0d35ydwqzd758l_w0000gn/T"

After my change:

(lldb) po packageDirectory
"/"

Since the manifest we're being told to load is /Package.swift, it seems like the result after my changes is the actual correct one. I think the way the test works today was actually done because of the issue I am fixing here, since the packageDirectory was previously an ever changing temporary directory path.

@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

@swift-ci please smoke test

@neonichu neonichu force-pushed the remove-manifest-loading-optimization branch from a2f1317 to 198a654 Compare November 2, 2022 18:49
@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

Forgot to actually push my changes before re-running 🤦

@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

@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))
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swiftpm/Sources/PackageLoading/ManifestLoader.swift:450:21: error: cannot find 'VFSOverlay' in scope
                try VFSOverlay(roots: [
                    ^~~~~~~~~~
/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swiftpm/Sources/PackageLoading/ManifestLoader.swift:451:21: error: cannot find 'VFSOverlay' in scope
                    VFSOverlay.File(name: manifestPath.pathString, externalContents: manifestTempFilePath.pathString)
                    ^~~~~~~~~~
[20/31][ 64%][31.370s] Linking Swift static library lib/libPackageCollections.a

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.
@neonichu neonichu force-pushed the remove-manifest-loading-optimization branch from 198a654 to bd6e3fd Compare November 2, 2022 20:17
@neonichu
Copy link
Contributor Author

neonichu commented Nov 2, 2022

@swift-ci please smoke test

@neonichu neonichu merged commit 3c692dc into main Nov 2, 2022
@neonichu neonichu deleted the remove-manifest-loading-optimization branch November 2, 2022 22:28
MaxDesiatov pushed a commit that referenced this pull request Nov 4, 2022
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.
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.

3 participants