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.

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

Conan improvements #4541

Merged
merged 9 commits into from
Oct 5, 2021
Merged

Conan improvements #4541

merged 9 commits into from
Oct 5, 2021

Conversation

mnonnenmacher
Copy link
Member

Various improvements for the Conan package manager:

See the commit messages for details.

Prefer "parse" over "extract" to align the function naming in Bower,
Cargo, and Conan with the other package manager implementations.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Using the `Stack` class was completely unnecessary and only made the
code more complex.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
`forEach` can be used directly as Kotlin automatically creates an
iterator for iterable objects.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
@mnonnenmacher mnonnenmacher requested a review from a team as a code owner October 4, 2021 23:23
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
@fviernau
Copy link
Member

fviernau commented Oct 5, 2021

Out of scope: Seeing these VCS URLs pointing to Conan index raises the question to me whether it is ok to return VCS URLs which do not point to the source code of the artifact. Actually I believe the answer to that should be a clear NO, as it creates the false impression that sources have been scanned and is inconsistent with ORTs approach. How about making the URLs just "bank" in a following task? Maybe @oss-review-toolkit/core-devs ?

@sschuberth
Copy link
Member

sschuberth commented Oct 5, 2021

Seeing these VCS URLs pointing to Conan index

Note that after the last commit in this PR, no VCS URL points to Conan index anymore.

whether it is ok to return VCS URLs which do not point to the source code of the artifact.

I'd also say "no", but this also is not the case anymore after the last commit in this PR. VCS URLs are either empty in favor of the real source artifact, or VCS info is deduced from the homepage, if possible.

fviernau
fviernau previously approved these changes Oct 5, 2021
analyzer/src/main/kotlin/managers/Conan.kt Outdated Show resolved Hide resolved
Running "conan inspect" takes about 1s each time. Calling it separately
for each field is therefore a huge waste of time, especially since the
same fields are requested multiple times for the same packages while
building the dependency tree.

To fix this, run "conan inspect" only once for each package and cache
the results for all fields. This requires to create a temporary JSON
file, because "conan inspect" does not support to output the JSON to
stdout.

This reduces the execution time of the `ConanFunTest` from about 100s to
about 30s and should have an even larger impact on projects with more
dependencies, especially if there are repetitions within the dependency
tree.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Use a dependency with one more level of transitive dependencies to
better show the issues with building the dependency tree which will be
fixed in the next commit.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Fix two issues with building the dependency tree:

* The algorithm wrongly used all nodes in the output of "conan info" as
  root nodes of the dependency, but only the first node represents the
  root.
* The dependencies for different scopes were assigned to separate
  `PackageReference` objects with the same id and afterwards added to a
  set. As a result only one of them was contained in the set.

The result was that the root level of the dependency tree contained too
many dependencies, while transitive dependencies were missing.

While at it, rename some variables for better readability.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Do not parse the VCS info from the output of "conan info", because this
only ever points to the recipe of the package, but not to the source
code. Usually this points to the ConanCenter index [1].

Instead, try to parse the VCS info from the homepage URL of the package.

This partly addresses #2037.

[1] https://github.com/conan-io/conan-center-index

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Try to parse the source artifact information for packages from the
conandata.yml file in the local repository.

Resolves #2037.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
@mnonnenmacher mnonnenmacher enabled auto-merge (rebase) October 5, 2021 11:28
@mnonnenmacher mnonnenmacher merged commit f3c3f4c into master Oct 5, 2021
@mnonnenmacher mnonnenmacher deleted the conan-improvements branch October 5, 2021 11:44
mnonnenmacher added a commit that referenced this pull request Apr 28, 2022
The related issue has been fixed in #4541.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
mnonnenmacher added a commit that referenced this pull request Apr 28, 2022
The related issue has been fixed in #4541.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
mnonnenmacher added a commit that referenced this pull request Apr 28, 2022
The related issue has been fixed in #4541.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants