Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(PurlUtils): Add missing Debian Purl type to getPurlType #7620

Closed
wants to merge 1 commit into from
Closed

fix(PurlUtils): Add missing Debian Purl type to getPurlType #7620

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions model/src/main/kotlin/utils/PurlUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
fun Identifier.getPurlType() =
when (type.lowercase()) {
"bower" -> PurlType.BOWER
"deb" -> PurlType.DEBIAN

Check warning on line 68 in model/src/main/kotlin/utils/PurlUtils.kt

View check run for this annotation

Codecov / codecov/patch

model/src/main/kotlin/utils/PurlUtils.kt#L68 

Added line #L68 was not covered by tests
Copy link
Member

@fviernau fviernau Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please indicate why you think this value is missing? (I believe so far this mapping was limited keys which are actually used by package managers within ORT.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At Bosch, we have a package manager plugin for Debian.
It used previously createPurl and I plan to migrate to toPurl following the merge of #7601.
toPurl calls this getPurlType function.

You wrote that getPurlType is meant to contains only package managers supported by ORT. In this case, we have a proprietary package manager plugin. How do you see it coexist with the rule you described in your comment ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You wrote that getPurlType is meant to

Actually, I didn't mean "is meant to" but I meant: it currently only contains values from the package managers in ORT.

How do you see it coexist with the rule you described in your comment ?

ORT has a mechanism to plug-in package managers, but lacks a mechanism for plugging in logic related to the ecosystem that package manager uses, e.g. Identifier.type. In my view, that should be a plug-in as well for the exact same reason the package manager has been chosen to be a plugin. However, we do not have such mechanism yet.

There are further places in the code which would need to be considered.
For example, OSV has a mapping from Identifier.type to OSV ecosystem constants. So, currently OSV cannot be used for plugin package managers which would require extension of that mapping.

In my view, it would make sense to define start a dedicated PackageManagagerPlugin interface, which would contain (very roughly spkeaking):

  1. The existing factory method, currently called PackageManagerFactory.create()
  2. A function which gives the purlType to the corresponding type (optional to implement)
  3. A function which gives the OSV ecosystem to the corresponding type

What do other guys think? @oss-review-toolkit/core-devs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, we do not have such mechanism yet.

Exactly. We had discussed that in past, and I have several branches on roughly that topic lying around (identifier-types, multiple-identifier-classes) that also try to deal with the problem that a package manager's project type does not need to match with it package type(s).

So for package manager that support multiple package types, like Pub, we'd even need a set of package types / PURLs / OSV ecosystem coordinates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sschuberth not sure if I get your point. Do you believe it's fine to add mappings for non-existing types now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if I get your point.

I was just amending to your API proposal more cases that we'd need to cover with such a new API.

Do you believe it's fine to add mappings for non-existing types now?

Basically no, but the right way to do it would require a rather large refactoring, I believe. Not sure whether that's feasible to do (now). I guess we should discuss this in a Kotlin developer meeting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2 cents: for the time being I found a workaround so we can close this PR.
However this topic should definitively be discussed as it will surely arise again.

"carthage" -> PurlType.CARTHAGE
"composer" -> PurlType.COMPOSER
"conan" -> PurlType.CONAN
Expand Down