oss-review-toolkit / ort

A suite of tools to automate software compliance checks.
https://oss-review-toolkit.org
Apache License 2.0
1.57k stars 308 forks source link

Implement helper function to get curations/package configurations path based on package id #5148

Open tsteenbe opened 2 years ago

tsteenbe commented 2 years ago

Propose we implement two helper functions getCurationFilePath(packageId: Identifier, fileExtension: String) and getPackageConfigurationsFilePath(packageId: Identifier) which would respectively return the full path to curations file (curations.yml or curations/{Package Type}/{Package Namespace}/{Package Name}.{fileExtension}) or package configurations (packages.yml or (package-configurations/ {Package Type}/{Package Namespace}/{Package Name}/{Package Version}/{vcs or sourceartifacts}.{fileExtension}).

Would like to use above helper functions in rules.kts but also noticed ORT already includes several functions that do something similar (see examples below) so I argue it better to have the helper functions in a common place instead of included in individual components

https://github.com/oss-review-toolkit/ort/blob/a531ccb99e66d8bd035867524c52a5033a47b63c/helper-cli/src/main/kotlin/common/Utils.kt#L133-L136

internal fun getSplitCurationFile(parent: File, packageId: Identifier, fileExtension: String) =
    parent.resolve(packageId.type.encodeOrUnknown())
        .resolve(packageId.namespace.ifBlank { "_" }.fileSystemEncode())
        .resolve("${packageId.name.encodeOrUnknown()}.$fileExtension")

https://github.com/oss-review-toolkit/ort/blob/a531ccb99e66d8bd035867524c52a5033a47b63c/model/src/main/kotlin/Identifier.kt#L123-L124

    fun toPath(separator: String = "/", emptyValue: String = "unknown"): String =
        sanitizedComponents.joinToString(separator) { it.encodeOr(emptyValue) }
tsteenbe commented 2 years ago

@fviernau In the ORT developer meeting of March 10, 2022 this was discussed but we did not make a decision as @mnonnenmacher note that you previously objected to have these kind of helper functions. Could you elaborate what are your objections?

fviernau commented 2 years ago

@tsteenbe I'm not sure what objections of mine you're referring to, can you give a hint (or @mnonnenmacher )? Besides that, I don't have any objections for these functions except for that I believe that for the --package-curations-dir it's good to accept any possible file organization scheme, e.g. that it works also if a curation is not in the file path getCurationFilePath() returns.

Maye this could be addressed by naming it getDefaultCurationFilePath()?

sschuberth commented 2 years ago

it's good to accept any possible file organization scheme

AFAIU this is exactly the objection. The proposal here is to require a fixed file organization scheme, and have a helper command to create that fixed scheme.

fviernau commented 2 years ago

The proposal here is to require a fixed file organization scheme

From the ticket description, I understood the proposal was to "just extract the helper functions so that the path can be used in the how to fix text". It does not say anything about that this file structure becomes mandatory. So, are you guys aligned (from the mentioned discussion in the developer meeting) that this ticket is about enforcing the file organization scheme?

sschuberth commented 2 years ago

So, are you guys aligned (from the mentioned discussion in the developer meeting) that this ticket is about enforcing the file organization scheme?

IMO yes. @tsteenbe and @mnonnenmacher can agree by 👍🏻 this comment 😉

mnonnenmacher commented 2 years ago

@tsteenbe See for example this discussion for more context: https://github.com/oss-review-toolkit/ort/pull/4966#discussion_r785795456

fviernau commented 2 years ago

IMO yes. @tsteenbe and @mnonnenmacher can agree by 👍🏻 this comment wink

You all seem to agree on that one. Mind providing a reason why you think the structure should be enforced?

Anyhow, my objections as you initially asked @tsteenbe boil down to the following underlying two bullet points. To me it seems it's introducing a limitation while the benefit / need is unclear.

  1. If we extend id matching of curations to be a bit more broad (like e.g. matching just type and namespace, but wildcard name and version) then it becomes unclear
  2. Flexibility allows for different use cases and preferences more easily, e.g.:
    • Some users may prefer a different file split
      • curations/provenance.yml
      • curations/declared-licenses.yml
      • curations/concluded-licenses.yml
    • Have a curations directory with symlinks to multiple curation dirs
      • curations/ort/ -> ort-config/curations
      • curations/your-org -> your-orgs/curations
    • Separate 100% correct curations from opinionated or maybe a little risky ones
      • curations/no-risk
      • curations/opinionated
sschuberth commented 2 years ago

Mind providing a reason why you think the structure should be enforced?