-
Notifications
You must be signed in to change notification settings - Fork 7
Adds swift package manager manifest support (one of many) #354
Conversation
Leaving change-log till approval. Will only merge after fetcher PR is merged. |
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.
One major structural question about SwiftPackageGitDep
, let dial that in before more review. Added some other comments as well.
Addressed in 82b04de |
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 code looks great but I have a few questions around SwiftType
and version constraints
toConstraint (From f) = CEq $ "^" <> f | ||
toConstraint (UpToNextMajor c) = CEq $ "^" <> c | ||
toConstraint (UpToNextMinor c) = CEq $ "~" <> c | ||
toConstraint (ClosedInterval (lhs, rhs)) = CEq $ ">=" <> lhs <> " " <> "<=" <> rhs | ||
toConstraint (RhsHalfOpenInterval (lhs, rhs)) = CEq $ ">=" <> lhs <> " " <> "<" <> rhs |
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.
I'm not entirely sure how to handle these version constraint types, but we're using CEq
incorrectly here. I think for From
you can use CCompatible
but we don't have an equivalent for the others yet...
I'm split between saying
- Strip all of the version constraints and leave the version as
CEq c
orCEq rhs/lhs
to ensure that the FOSSA backend can handle the version correctly. - Leave it the way you have it because its more correct.
Can you test what happens when a locator is sent to the backed with a RhsHalfOpenInterval
? I'm concerned that the result won't be what we want. It may be better to do CEq lhs
with a note about how we have an issue. You should be able to make a fossa-deps file, type: git
, name: <some swift package with the valid constraint>
.
[future-work] Alternatively, you could add a type like CRhsHalfOpenInterval
to VerConstraint
so that we handle this properly when the graph is converted to SrcUnits.
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.
My preference is we go with (2) leave it way it is because it is more correct.
If (1) is followed, we might fail to resolve versions when LHS or RHS of constraint does not exist (not a perfect example but ~ e.g. >= 0.0.0, where 0.0.0 does not exist)
I've validated both RhsHalfOpenInterval
, ClosedInterval
, and others in unit and integration test for swift fetchers:
- (integration test in fetcher) https://github.com/fossas/FOSSA/pull/6418/files#diff-afd29de33fe5b6b70a89eaaafe634ffae1c7a94d3d081ed77522295b625a942dR83
- (spectrometer unit test) - https://github.com/fossas/spectrometer/pull/354/files#diff-68e91f6fa560fd2d45ebf74cd2eacc278a5fff0d66e2a7d218e9480661a5ae48R71
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.
I'm comfortable with option 2 as well, mainly because of our conversation yesterday about how the Swift fetcher is built to handle these constraints.
Could we actually add a type similar to CRhsHalfOpenInterval
and others for this PR? I think that would ease all of my concerns about version handling and it shouldn't be a hard addition. We're going to need it eventually and I think this VersionConstraint
would make sure we always handle this version case the same everywhere.
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 detailed answers! Looks great.
13a09fb
to
08db396
Compare
Overview
Adds analyses for swift package manager manifest.
Acceptance criteria
Testing plan
fossa analyze -o
, and inspect the result.To perform, end to end analysis,
Risks
N/A
References
Works on https://github.com/fossas/team-analysis/issues/711
Checklist
docs/
.Changelog.md
if this change is externally facing. If this PR did not mark a release, I added my changes into an# Unreleased
section at the top.