Update to new SwiftPM Workspace initializer #458
Conversation
|
@tomerd this adopts the new "customToolchain" parameter to the Workspace that you added. I had to remove our customMirrors setup, because I didn't see a public initializer that takes both customMirrors and customToolchain. Maybe we should add customMirrors to this newer initializer? I'm also not completely clear how our mirrors setup differs from the default. The last comment on this from you #427 (comment) implied that we still need it. Is that still true? The default implementation of mirrors looks reasonable to me, but it's a bit hard to trace through exactly how it would differ from what we're currently doing. |
| sharedMirrorFile: location.sharedMirrorsConfigurationFile, | ||
| fileSystem: fileSystem | ||
| ) | ||
| let configuration = WorkspaceConfiguration( |
There was a problem hiding this comment.
var configuration = WorkspaceConfiguration.default
configuration.skipDependenciesUpdates: true // <-- example of customization of the default
potentially more future facing support of defaults
| resolverUpdateEnabled: false | ||
| configuration: configuration, | ||
| customHostToolchain: toolchain, | ||
| customManifestLoader: ManifestLoader(toolchain: toolchain.configuration, cacheDir: location.workingDirectory) |
There was a problem hiding this comment.
I wonder if we still need a custom manifest loader now that we are providing a custom toolchain.
ManifestLoader has only two argument: the toolchain (which we now provide explicitly) and the cacheDir. I think in the case of LSP we can use the default SwiftPM cache dir so that would mean we can just drop this argument.
|
looks good. some ideas inline |
I think the way you have it is correct. the old initializers there was no link between the location and the mirrors, but that ws part of the changes that led up to the new initializers |
* Pass in our custom toolchain * Remove custom mirror config; swiftpm's default should now match our specified location. * Remove custom manifest loader; swiftpm's default uses our toolchain and should work now.
benlangmuir
force-pushed
the
update-swiftpm-init
branch
from
925d88e to
dc5ce31
Compare
|
Thanks @tomerd, updated based on your suggestions |
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test Linux |
|
@swift-ci please test macOS |
|
What did the mirrors configuration do? I looked inside the SwiftPM repo but couldn’t find any documentation on it. |
|
It was added here: #299 I believe that it is no longer necessary (a) because the API we're using will do the right thing internally, and (b) we shouldn't be triggering updates anymore. |
|
correct, we now have SwiftPM handle this detail internally base on the Location that is provided by the client, and in this case (most cases) initialized to the the package root |
No description provided.