refactor(poetry): Improve the definition file paths #7624
Conversation
fviernau
force-pushed
the
poetry-definition-file-name
branch
from
a11f3b1 to
c2a6eba
Compare
In order to do it's job, `Poetry.kt` generates a requirements file and then relies on `Pip.kt` to do the analysis. The resulting `ProjectAnalyzerResult`s uses the paths to these generated requirements files as definiton file paths. This does not make sense, because: 1. The generation of the files is an implementation detail. 2. The generated files do not exist in the code repository which can lead to confusion, in particular also these get referenced from ORT configuration / path excludes. Fix that by patching up the `ProjectAnalyzerResult`s to recover the original definition file name. Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
poetry-definition-file-name
branch
from
c2a6eba to
8882bf0
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7624 +/- ##
=========================================
Coverage 68.03% 68.03%
Complexity 2022 2022
=========================================
Files 344 344
Lines 16725 16725
Branches 2371 2371
=========================================
Hits 11379 11379
Misses 4363 4363
Partials 983 983
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| .map { projectAnalyzerResult -> | ||
| projectAnalyzerResult.copy( | ||
| project = projectAnalyzerResult.project.copy( | ||
| definitionFilePath = VersionControlSystem.getPathInfo(definitionFile).path |
There was a problem hiding this comment.
I propose definitionFile.relativeTo(analysisRoot) instead to avoid adding more code that relies on the project to be version controlled.
There was a problem hiding this comment.
This does not introduce the requirement, because code which computes the value for definitionFilePath already relies on that prior to this change. Furthermore, relativeTo(analysisRoot) seems wrong because the definition file path is not meant to be relative to the analysis root, but the repository root.
There was a problem hiding this comment.
This does not introduce the requirement
And I didn't say that. I was talking about adding more code with that requirement.
Furthermore,
relativeTo(analysisRoot)seems wrong because the definition file path is not meant to be relative to the analysis root, but the repository root.
Indeed Project.definitionFilePath is documented like that 😞 IMO that's a mistake already, and it will make our lives harder in order to address #2896.
Anyway, we already have a bunch of code like that, so we can bulk-address this later, I guess.
There was a problem hiding this comment.
And I didn't say that. I was talking about adding more code with that requirement.
I know, I didn't mean to say that you said that. I only intended to clarify that both, the old and the new code path depend on that same logic.
In order to do it's job,
Poetry.ktgenerates a requirements file and then relies onPip.ktto do the analysis. The resultingProjectAnalyzerResults uses the paths to these generated requirements files as definiton file paths. This does not make sense, because:Fix that by patching up the
ProjectAnalyzerResults to recover the original definition file name.