refactor: Use environments to install Conan #10127

heliocastro · 2025-04-03T14:39:36Z

Move previous conan/conan2 naming approach to Python environment solution. A wrapper that detects CONAN_SERIES environment var switchis between the two release series. Defaults to CONAN_SERIES=2.
Enable profile creation support by default Passing environment var CONAN_CREATE_PROFILE=false will prevent to generate a default profile.

This is somehow how it works:

Dockerfile

+RUN eval "$(pyenv init - bash)"  \
+    && eval "$(pyenv virtualenv-init -)" \
+    && pyenv virtualenv conan \
+    && pyenv activate conan \
+    && pip install conan==${CONAN_VERSION} \
+    && pyenv deactivate \
+    && pyenv virtualenv conan2 \
+    && pyenv activate conan2 \
+    && pip install conan==${CONAN2_VERSION} \
+    && pyenv deactivate \
+    && sudo chmod +x ${PYENV_ROOT}/bin/conan


Dockerfile

+RUN eval "$(pyenv init - bash)"  \
+    && eval "$(pyenv virtualenv-init -)" \
+    && pyenv virtualenv conan \
+    && pyenv activate conan \
+    && pip install conan==${CONAN_VERSION} \
+    && pyenv deactivate \
+    && pyenv virtualenv conan2 \
+    && pyenv activate conan2 \
+    && pip install conan==${CONAN2_VERSION} \
+    && pyenv deactivate \
+    && sudo chmod +x ${PYENV_ROOT}/bin/conan


codecov · 2025-04-03T15:16:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.72%. Comparing base (034d082) to head (76b591b).
Report is 12 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #10127      +/-   ##
============================================
- Coverage     56.76%   56.72%   -0.04%     
  Complexity     1640     1640              
============================================
  Files           337      337              
  Lines         12469    12484      +15     
  Branches       1172     1178       +6     
============================================
+ Hits           7078     7082       +4     
- Misses         4940     4951      +11     
  Partials        451      451

Flag	Coverage Δ
funTest-docker	`71.03% <ø> (+0.08%)`	⬆️
funTest-non-docker	`33.02% <ø> (ø)`
test-ubuntu-24.04	`41.06% <ø> (-0.02%)`	⬇️
test-windows-2022	`41.04% <ø> (-0.02%)`	⬇️

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.

nnobelis · 2025-05-28T04:47:51Z

@heliocastro Kind reminder to look at this. AFAIK, there are only the tests to fix.

I think this is an important change that should be in ORT.

sschuberth · 2025-05-28T06:17:35Z

plugins/package-managers/conan/src/main/kotlin/Conan.kt

+            workingDir,
+            environment + mapOf(
+                "CONAN_NON_INTERACTIVE" to "1",
+                "CONAN_SERIES" to if (useConan2) "2" else "1"


Can we rename CONAN_SERIES to e.g. CONAN_MAJOR_VERSION or so? "Series" sounds a bit uncommon to me.

nnobelis · 2025-06-04T15:55:54Z

@heliocastro I fixed the shims error (with the help of pyenv/pyenv#1157 (comment)).
I open the PR for review, I how this is ok for you.

I will do tomorrow the fix required by @sschuberth.

heliocastro · 2025-06-04T18:27:15Z

Perfectly fine. Sorry for unable to get on it sooner as I have a crazy workload at work.
Many thanks for the help

- Move previous conan/conan2 naming approach to Python environment solution. A wrapper that detects CONAN_SERIES environment var switchis between the two release series. Defaults to CONAN_SERIES=2. - Enable profile creation support by default Passing environment var CONAN_CREATE_PROFILE=false will prevent to generate a default profile. Signed-off-by: Helio Chissini de Castro <[email protected]> Signed-off-by: Nicolas Nobelis <[email protected]>

…nments Signed-off-by: Nicolas Nobelis <[email protected]>

…efault This new profile is created with `-detect` therefore it will contains the compilation flags. This is cleaner as it allows to remove the passing of the compilation flags from the package manager. It also fix the usage of Conan on non-Linux plaforms. Signed-off-by: Nicolas Nobelis <[email protected]>

- Remove DUMMY_COMPILER_SETTINGS from plugin code as breaks any non-linux systems. Signed-off-by: Helio Chissini de Castro <[email protected]> Signed-off-by: Nicolas Nobelis <[email protected]>

sschuberth · 2025-06-05T06:45:29Z

I will do tomorrow the fix required by @sschuberth.

As agreed with @nnobelis this will not go into today's release yet.

This is an alignment commit to reflect the change in ORT due to [1]. [1]: oss-review-toolkit/ort#10127 Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis · 2025-06-06T11:18:50Z

@sschuberth Can we merge this now ?

sschuberth

While the changes basically seem to be good, I'm missing an overarching rationale explaining what actual issues venvs solve. Is it about being able to properly detect profile settings in isolation?

sschuberth · 2025-06-06T11:43:53Z

Dockerfile

@@ -136,7 +136,7 @@ ARG PYTHON_VERSION
 ARG PYENV_GIT_TAG

 ENV PYENV_ROOT=/opt/python
-ENV PATH=$PATH:$PYENV_ROOT/shims:$PYENV_ROOT/bin:$PYENV_ROOT/conan2/bin
+ENV PATH=$PATH:$PYENV_ROOT/shims:$PYENV_ROOT/bin


IMO the commit message needs to be reworked. To me, using dashes for a list / an enumeration is always an alarm sign that probably too much work has been crammed into a single commit.

I propose something like:

Migrate the previous approach to install binaries named `conan` and `conan2` to properly separated virtualenvs for Python. This is done via the pyenv-virtualenv plugin [1] for pyenv [2]. [1]: https://github.com/pyenv/pyenv-virtualenv [2]: https://github.com/pyenv/pyenv

sschuberth · 2025-06-06T11:45:44Z

plugins/package-managers/conan/src/main/kotlin/Conan.kt

@@ -79,7 +79,7 @@ internal class ConanCommand(private val useConan2: Boolean = false) : CommandLin
    override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.44.0 <3.0")

    override fun run(vararg args: CharSequence, workingDir: File?, environment: Map<String, String>) =
-        super.run(args = args, workingDir, environment + ("CONAN_NON_INTERACTIVE" to "1"))
+        super.run(args = args, workingDir, environment + mapOf("CONAN_NON_INTERACTIVE" to "1", "CONAN_SERIES" to "1"))


This still refer to CONAN_SERIES instead of CONAN_MAJOR_VERSION. But is it even correct to hard-code this here? Shouldn't it be dropped to make run work for both major versions?

sschuberth · 2025-06-06T11:57:43Z

scripts/setup_conan.sh

+if [[ "$conan_option" -eq 1 ]]; then # Setting up Conan 1.x series
+    pyenv activate conan
+    # Docker has modern libc
+    CONAN_RECURSIVE_CALL=1 conan profile update settings.compiler.libcxx=libstdc++11 ort-default


Why does this only need to be done for Conan 1?

sschuberth · 2025-06-06T12:00:42Z

scripts/setup_conan.sh

+# License-Filename: LICENSE
+#
+
+conan_option=${CONAN_MAJOR_VERSION:-2}


How about making this

CONAN_VENV="conan${CONAN_MAJOR_VERSION:-2}"

and then below simply call

pyenv activate $CONAN_VENV

once?

However, that would require to rename the venv from "conan" to "conan1", but maybe it's not a bad ideal to be explicit anyway.

sschuberth · 2025-06-06T12:06:40Z

scripts/setup_conan.sh

+
+# Since this script is installed with the name "conan", there is a risk of infinite recursion if pyenv is not available
+# on the PATH, which can occur when setting up a development environment. To prevent this, check for recursive calls.
+if [[ "$CONAN_RECURSIVE_CALL" -eq 1 ]]; then


A simpler check could be to compare ${BASH_SOURCE[1]} with this script's path, i.e. ${BASH_SOURCE[0]}.

sschuberth · 2025-06-06T12:08:23Z

plugins/package-managers/conan/src/main/kotlin/ConanV1Handler.kt

@@ -44,6 +44,12 @@ import org.ossreviewtoolkit.utils.ort.createOrtTempDir
 internal class ConanV1Handler(private val conan: Conan) : ConanVersionHandler {
    override fun getConanHome(): File = Os.userHomeDirectory.resolve(".conan")

+    override fun createConanProfileIfNeeded() {
+        if (!getConanHome().resolve("profiles/ort-default").isFile) {
+            conan.command.run("profile", "new", "ort-default", "--detect").requireSuccess()


The "ort-default" should only be added to the script in this commit which makes use of it.

sschuberth · 2025-06-06T12:09:22Z

plugins/package-managers/conan/src/main/kotlin/Conan.kt

@@ -222,6 +222,8 @@ class Conan(
                config.lockfileName?.let { hasLockfile(workingDir.resolve(it).path) } == true
            }

+            handler.createConanProfileIfNeeded()


Commit message: s/-detect/--detect/

sschuberth · 2025-06-06T12:11:22Z

plugins/package-managers/conan/src/main/kotlin/Conan.kt

@@ -122,12 +122,6 @@ class Conan(
    private val config: ConanConfig
 ) : PackageManager("Conan") {
    companion object {
-        internal val DUMMY_COMPILER_SETTINGS = arrayOf(


I think it's fine (and even less confusing) to squash this to the previous commit, saying to replace hard-coded dummy compiler settings with proper ones. Otherwise there's an intermediate commit with both dummy and detected settings, which does not make sense.

github-advanced-security bot found potential problems Apr 3, 2025

View reviewed changes

heliocastro force-pushed the feat/one_conan_to_rule_then_all branch from aa2e6e1 to 17e8541 Compare April 3, 2025 14:40

heliocastro self-assigned this Apr 3, 2025

heliocastro added the docker About Docker topics label Apr 3, 2025

heliocastro requested a review from nnobelis April 3, 2025 14:41

heliocastro force-pushed the feat/one_conan_to_rule_then_all branch from 17e8541 to 6953358 Compare April 3, 2025 14:59

heliocastro force-pushed the feat/one_conan_to_rule_then_all branch 2 times, most recently from 9a52c9c to 7ecc19b Compare April 4, 2025 06:22

nnobelis force-pushed the feat/one_conan_to_rule_then_all branch 2 times, most recently from c4ad09c to 5976150 Compare April 10, 2025 11:20

This comment was marked as outdated.

Sign in to view

nnobelis force-pushed the feat/one_conan_to_rule_then_all branch 2 times, most recently from d98bb1e to d95e56a Compare April 10, 2025 11:37

This comment was marked as outdated.

Sign in to view

sschuberth reviewed May 28, 2025

View reviewed changes

nnobelis force-pushed the feat/one_conan_to_rule_then_all branch 6 times, most recently from a83a763 to e5b7543 Compare June 4, 2025 15:53

nnobelis marked this pull request as ready for review June 4, 2025 15:55

nnobelis requested a review from a team as a code owner June 4, 2025 15:55

heliocastro and others added 3 commits June 5, 2025 07:50

fix(conan): Adapt the Conan package manager to use the virtual enviro…

321ae2a

…nments Signed-off-by: Nicolas Nobelis <[email protected]>

refactor(conan): Remove dummy profile settings

76b591b

- Remove DUMMY_COMPILER_SETTINGS from plugin code as breaks any non-linux systems. Signed-off-by: Helio Chissini de Castro <[email protected]> Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis force-pushed the feat/one_conan_to_rule_then_all branch from e5b7543 to 76b591b Compare June 5, 2025 09:10

nnobelis requested a review from sschuberth June 5, 2025 09:10

nnobelis mentioned this pull request Jun 5, 2025

refactor(analyzer): Use environments to install Conan eclipse-apoapsis/ort-server#2890

Draft

sschuberth requested changes Jun 6, 2025

View reviewed changes

sschuberth assigned nnobelis Jun 18, 2025

sschuberth mentioned this pull request Jun 18, 2025

fix(conan): Properly check for Conan version #10493

Closed

refactor: Use environments to install Conan #10127

Are you sure you want to change the base?

refactor: Use environments to install Conan #10127

Uh oh!

Conversation

heliocastro commented Apr 3, 2025

Uh oh!

Check warning

Check warning

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!

This comment was marked as outdated.

This comment was marked as outdated.

nnobelis commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnobelis commented Jun 4, 2025

Uh oh!

heliocastro commented Jun 4, 2025

Uh oh!

sschuberth commented Jun 5, 2025

Uh oh!

nnobelis commented Jun 6, 2025

Uh oh!

sschuberth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Apr 3, 2025 •

edited

Loading

nnobelis commented May 28, 2025 •

edited

Loading