fix(swiftpm): Use `identity` as `name` for Swift PM registry dependencies #10124

liedQM · 2025-04-03T06:57:40Z

Currently only the location field is used to derive the name of a Swift PM dependency. For dependencies fetched over a Package Registry Service (Swift PM registry) the location field is empty which leads to incomplete and invalid results. For those dependencies the identity needs to be used instead.

Technical notes:

Screenshots of a broken and fixed scan with SPM registries involved:

broken:

fixed:

Note: Like seen, the scan also stops resolving the broken SPM registries further which results in much shorter scan results.

Test outcome of the newly added test without the fix:
expected:

project:
  id: "SwiftPM::src/funTest/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency/Package.resolved:e1d45fb08d20491de87deb7decd464633707ebaf"
  definition_file_path: "plugins/package-managers/swiftpm/src/funTest/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency/Package.resolved"
  declared_licenses: []
  declared_licenses_processed: {}
  vcs:
    type: ""
    url: ""
    revision: ""
    path: ""
  vcs_processed:
    type: "Git"
    url: "https://github.com/oss-review-toolkit/ort.git"
    revision: "e1d45fb08d20491de87deb7decd464633707ebaf"
    path: "plugins/package-managers/swiftpm/src/funTest/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency"
  homepage_url: ""
  scopes:
  - name: "dependencies"
    dependencies:
    - id: "Swift::alamofire.alamofire:5.4.4"
packages:
- id: "Swift::alamofire.alamofire:5.4.4"
  purl: "pkg:swift/[email protected]"
  declared_licenses: []
  declared_licenses_processed: {}
  description: ""
  homepage_url: ""
  binary_artifact:
    url: ""
    hash:
      value: ""
      algorithm: ""
  source_artifact:
    url: ""
    hash:
      value: ""
      algorithm: ""
  vcs:
    type: ""
    url: ""
    revision: ""
    path: ""
  vcs_processed:
    type: ""
    url: ""
    revision: ""
    path: ""

but was:

project:
  id: "SwiftPM::src/funTest/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency/Package.resolved:e1d45fb08d20491de87deb7decd464633707ebaf"
  definition_file_path: "plugins/package-managers/swiftpm/src/funTest/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency/Package.resolved"
  declared_licenses: []
  declared_licenses_processed: {}
  vcs:
    type: ""
    url: ""
    revision: ""
    path: ""
  vcs_processed:
    type: "Git"
    url: "https://github.com/oss-review-toolkit/ort.git"
    revision: "e1d45fb08d20491de87deb7decd464633707ebaf"
    path: "plugins/package-managers/swiftpm/src/funTest/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency"
  homepage_url: ""
  scopes:
  - name: "dependencies"
    dependencies:
    - id: "Swift::null:5.4.4"
packages:
- id: "Swift::null:5.4.4"
  purl: "pkg:swift/[email protected]"
  declared_licenses: []
  declared_licenses_processed: {}
  description: ""
  homepage_url: ""
  binary_artifact:
    url: ""
    hash:
      value: ""
      algorithm: ""
  source_artifact:
    url: ""
    hash:
      value: ""
      algorithm: ""
  vcs:
    type: ""
    url: ""
    revision: ""
    path: ""
  vcs_processed:
    type: ""
    url: ""
    revision: ""
    path: ""

sschuberth · 2025-04-03T07:31:34Z

Thanks a lot for the contribution @liedQM! Please have a look at the failing commit linter: You should hard-wrap commit message body lines at column 75.

sschuberth · 2025-04-03T07:33:12Z

plugins/package-managers/swiftpm/src/main/kotlin/SwiftPm.kt

@@ -244,7 +244,12 @@ private fun PinV2.toId(): Identifier =
    Identifier(
        type = PACKAGE_TYPE,
        namespace = "",
-        name = getCanonicalName(location),
+        // For SPM registry dependencies the `location` field is blank -> Use `identity` instead


Nit: Please make this a sentence as "written in a book" (e.g. with a dot at the end):

// For SPM registry dependencies the `location` field is blank, so use the `identity` field instead.

Shall we move this comment to the location property?

Thanks for the sentence suggestion, I've applied it.

codecov · 2025-04-03T07:40:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.60%. Comparing base (86de3cc) to head (f3980be).
Report is 85 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #10124   +/-   ##
=========================================
  Coverage     69.60%   69.60%           
  Complexity     1454     1454           
=========================================
  Files           270      270           
  Lines          9668     9668           
  Branches       1028     1028           
=========================================
  Hits           6729     6729           
  Misses         2487     2487           
  Partials        452      452

Flag	Coverage Δ
funTest-docker	`68.80% <ø> (+0.02%)`	⬆️
funTest-non-docker	`33.32% <ø> (ø)`
test-ubuntu-24.04	`39.14% <ø> (ø)`
test-windows-2022	`39.12% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

...est/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency/Package.resolved

fviernau · 2025-04-03T08:15:48Z

plugins/package-managers/swiftpm/src/main/kotlin/SwiftPm.kt

@@ -244,7 +244,12 @@ private fun PinV2.toId(): Identifier =
    Identifier(
        type = PACKAGE_TYPE,
        namespace = "",
-        name = getCanonicalName(location),
+        // For SPM registry dependencies the `location` field is blank -> Use `identity` instead


Shall we move this comment to the location property?

liedQM · 2025-04-03T08:23:17Z

Thanks a lot for the contribution @liedQM! Please have a look at the failing commit linter: You should hard-wrap commit message body lines at column 75.

Thanks for the fast feedback. Just a short note: You refer to https://robertcooper.me/post/git-commit-messages in your Contribution Guideline which limits the commit messages to 80 characters (which I've followed). It would be nice to add a note in your guidelines regarding the smaller character count :)

sschuberth · 2025-04-03T08:39:31Z

Just a short note: You refer to https://robertcooper.me/post/git-commit-messages in your Contribution Guideline which limits the commit messages to 80 characters (which I've followed). It would be nice to add a note in your guidelines regarding the smaller character count :)

Thanks for letting us now, here's the proposed addition to the docs.

fviernau · 2025-04-03T11:11:48Z

commit-msg: Should say identity instead of identifier ?

liedQM · 2025-04-03T11:14:03Z

commit-msg: Should say identity instead of identifier ?

Thanks it seems I've mixed the terms up. Will fix it.

fviernau · 2025-04-03T11:15:17Z

.../assets/projects/synthetic/expected-output-only-lockfile-v3-with-SPM-registry-dependency.yml

+    - id: "Swift::alamofire.alamofire:5.4.4"
+packages:
+- id: "Swift::alamofire.alamofire:5.4.4"
+  purl: "pkg:swift/[email protected]"


question: What do you guys think about the purl? Is the current logic appropriate which sets it, as we're dealing with private registries, could this be ambigous?

could this be ambigous?

Theoretically it would be. If we can be sure the registry to be a non-default one, it's probably a good idea to add the repository_url qualifier to the purl.

could this be ambigous?

Theoretically it would be. If we can be sure the registry to be a non-default one, it's probably a good idea to add the repository_url qualifier to the purl.

We don't get any information regarding the used registry from the Package.resolved or Package.swift since they are defined outside the project using the swift package-registry set command for a specific scope, e.g. alarmofire (the one before the .) in the test example.

Note: The Package.swift entry for the test example would be:

.package( id: "alamofire.alamofire", from: 5.4.4 )

Currently only the `location` field is used to derive the `name` of a Swift PM dependency. For dependencies fetched over a Package Registry Service (Swift PM registry) the `location` field is empty which leads to incomplete and invalid results. For those dependencies the `identity` needs to be used instead. Signed-off-by: Marco Lied <[email protected]>

liedQM · 2025-04-07T09:40:17Z

Do you need anything from me for the PR to be merged? The missing registry support blocks us to switch to ORT since a lot of dependencies are missed in the scan.
I'll create another PR which adds target (different dependency types) support of SPM similar to maven which allows excluding test-only dependencies but that will take some time and a bigger PR to make it work.

sschuberth · 2025-04-07T10:18:01Z

Do you need anything from me for the PR to be merged?

@fviernau could you please do another review?

sschuberth · 2025-04-15T05:46:32Z

Do you need anything from me for the PR to be merged?

@fviernau could you please do another review?

@fviernau are you okay to merge this (or to dismiss your review)?

fviernau · 2025-04-15T09:00:42Z

@fviernau are you okay to merge this (or to dismiss your review)?

yes, I'm ok to merge this as a "partial" solution, even though identity may be only an SwiftPM-internal project-local unique id, for which it is questionable whether the string value is always suitable for use withing the ORT package identifier. However, it can be improved later on. (Same for the purl).

@liedQM just out of curiosity: Would you know whether SwiftPM has a command to retrieve metadata for such package from the remote registry. E.g. the name, description and so forth?

I'm ok with this.

liedQM · 2025-04-22T08:26:00Z

@fviernau currently I'm not aware of any way despite reading it out of the a global settings file. I've been looking into the available swift pm commands and haven't found any to read the current scope configuration yet.
Nevertheless the SPM registry works like the maven repositories where a dependency can be loaded from any registry which provides the library, so it can be multiple ones.
Are you saving this information for maven dependencies as well? If yes I can dig deeper after my scope fix PR but if not I would leave it out for SPM as well.

liedQM · 2025-04-22T08:26:10Z

Thanks for merging my PR :)

sschuberth · 2025-04-22T08:28:37Z

Are you saving this information for maven dependencies as well?

Yes, we do.

liedQM requested a review from a team as a code owner April 3, 2025 06:57

sschuberth requested changes Apr 3, 2025

View reviewed changes

fviernau reviewed Apr 3, 2025

View reviewed changes

...est/assets/projects/synthetic/only-lockfile-v3-with-SPM-registry-dependency/Package.resolved Show resolved Hide resolved

fviernau previously requested changes Apr 3, 2025

View reviewed changes

liedQM force-pushed the fix-spm-registry-support branch from d72561e to 37c2438 Compare April 3, 2025 11:07

liedQM changed the title ~~fix(swiftpm): Use identifier as name for Swift PM registry dependencies to~~ fix(swiftpm): Use identifier as name for Swift PM registry dependencies Apr 3, 2025

liedQM changed the title ~~fix(swiftpm): Use identifier as name for Swift PM registry dependencies~~ fix(swiftpm): Use identity as name for Swift PM registry dependencies Apr 3, 2025

fviernau reviewed Apr 3, 2025

View reviewed changes

liedQM force-pushed the fix-spm-registry-support branch from 37c2438 to f3980be Compare April 3, 2025 11:16

sschuberth approved these changes Apr 3, 2025

View reviewed changes

sschuberth requested a review from fviernau April 4, 2025 06:07

sschuberth merged commit 3e310dc into oss-review-toolkit:main Apr 15, 2025
25 checks passed

liedQM deleted the fix-spm-registry-support branch April 23, 2025 07:22

fix(swiftpm): Use identity as name for Swift PM registry dependencies #10124

fix(swiftpm): Use identity as name for Swift PM registry dependencies #10124

Uh oh!

Conversation

liedQM commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sschuberth commented Apr 3, 2025

Uh oh!

sschuberth Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

fviernau Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

liedQM Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

fviernau Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

liedQM commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sschuberth commented Apr 3, 2025

Uh oh!

fviernau commented Apr 3, 2025

Uh oh!

liedQM commented Apr 3, 2025

Uh oh!

fviernau Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

sschuberth Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liedQM Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

liedQM commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sschuberth commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sschuberth commented Apr 15, 2025

Uh oh!

fviernau commented Apr 15, 2025

Uh oh!

Uh oh!

liedQM commented Apr 22, 2025

Uh oh!

liedQM commented Apr 22, 2025

Uh oh!

sschuberth commented Apr 22, 2025

Uh oh!

Uh oh!

fix(swiftpm): Use `identity` as `name` for Swift PM registry dependencies #10124

fix(swiftpm): Use `identity` as `name` for Swift PM registry dependencies #10124

liedQM commented Apr 3, 2025 •

edited

Loading

codecov bot commented Apr 3, 2025 •

edited

Loading

liedQM commented Apr 3, 2025 •

edited

Loading

sschuberth Apr 3, 2025 •

edited

Loading

liedQM commented Apr 7, 2025 •

edited

Loading

sschuberth commented Apr 7, 2025 •

edited

Loading