fix package describe to handle binary targets #3810
Conversation
tomerd
requested review from
abertelrud,
friedbunny and
neonichu
as code owners
|
@swift-ci please smoke test |
Sources/Workspace/Workspace.swift
Outdated
|
|
||
| // radar/82263304 | ||
| // compute binary artifacts for the sake of constructing a project model | ||
| // note this does not actually download remote artifacts and as such does not have the artifact's correct path |
There was a problem hiding this comment.
@neonichu @abertelrud I dont love this, but its a tradeoff between needing to load the entire graph / download the binary artifacts as this API goal is to provide a quicker view into the package
There was a problem hiding this comment.
if we are okay with the approach we would need to make this DRYer with other other artifact computation code
There was a problem hiding this comment.
I think having a fake path is fine here.
I guess another option would be changing path into an enum with a case for an actual path and something like noYetDownloaded, but not sure if that is really worth the hassle.
There was a problem hiding this comment.
Right, in theory it seems that an enum is the way to go here in the future. Or the binary artifact should have an optional path. Or there should be some kind of distinction between known binary artifacts vs actually present binary artifacts. But in any case, I don't think it should block fixing the real issue addressed by this PR, so it shouldn't hold this up.
tomerd
added
the
ready
tomerd
changed the title
Sources/Workspace/Workspace.swift
Outdated
|
|
||
| // radar/82263304 | ||
| // compute binary artifacts for the sake of constructing a project model | ||
| // note this does not actually download remote artifacts and as such does not have the artifact's correct path |
There was a problem hiding this comment.
I think having a fake path is fine here.
I guess another option would be changing path into an enum with a case for an actual path and something like noYetDownloaded, but not sure if that is really worth the hassle.
tomerd
force-pushed
the
radar/82263304
branch
from
d74fa6a to
affcc48
Compare
|
@swift-ci please smoke test |
| throw StringError("invalid manifests at \(root.packages)") | ||
|
|
||
| let package = try tsc_await { | ||
| workspace.loadRootPackage( |
There was a problem hiding this comment.
Standardizing all the various tool actions on loading at least the root package first seems like a good thing, so this seems like great cleanup regardless.
Sources/Workspace/Workspace.swift
Outdated
|
|
||
| // radar/82263304 | ||
| // compute binary artifacts for the sake of constructing a project model | ||
| // note this does not actually download remote artifacts and as such does not have the artifact's correct path |
There was a problem hiding this comment.
Right, in theory it seems that an enum is the way to go here in the future. Or the binary artifact should have an optional path. Or there should be some kind of distinction between known binary artifacts vs actually present binary artifacts. But in any case, I don't think it should block fixing the real issue addressed by this PR, so it shouldn't hold this up.
motivation: package describe command fails for packages with binary targets changes: * PackageBuilder requires remote artifacts to be set to compute the model, as such make it a required argument * do not call PackageBuilder directly from describe and format commands, instead use Workspace::loadRootPackage API for loading a root package * update Workspace::loadRootPackage to compute artifacts
tomerd
force-pushed
the
radar/82263304
branch
from
3b762fe to
fd4e5ee
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
|
How can i test this version locally? as currently with latest xcode 14beta 3, we are getting swift package manager version 5.7. How can i update the SPM version locally to validate ? |
|
@tomerd |
|
the version of swift that ships with beta 3 should include this fix. are you still seeing an issue? |
motivation: package describe command fails for packages with binary targets
changes:
rdar://82263304