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

[service] Ability to purposefully NOT build all versions for a recipe #2813

Closed
Croydon opened this issue Sep 3, 2020 · 6 comments
Closed
Labels
infrastructure Waiting on tools or services belonging to the infra priority: high

Comments

@Croydon
Copy link
Contributor

Croydon commented Sep 3, 2020

Current situtation

Before we merged conan-io/hooks#223 we had an easy way to NOT build all versions for a recipe:

Add all versions to the conandata.yml but not the config.yml file.

I understand why this hook was added, as it was a common error to do this by accident, however, now we can't make such a purposeful decision anymore.

This is a problem which increases basically by every single day passing by. We don't want to build on C3I all versions for all packages forever, but it is nice to give people the chance to use the recipes without having to modify them first and still to empower them to build the respective versions locally.

Think about huge packages like Qt and Boost and projects which release many, many versions like CMake and OpenSSL. Building those will become increasingly heavy on C3I over time. Some already take many hours today.

For e.g. OpenSSL we have done this already in conan-community: Build only the latest 3 patch versions from each version series https://github.com/conan-community/conan-openssl/blob/master/.travis.yml

And when we ported OpenSSL to CCI we basically did the same for the initial versions. For the 1.0.2 and the 1.1.0 series only the last few patch versions are added to the config.yml while everything is added to conandata.yml

https://github.com/conan-io/conan-center-index/blob/master/recipes/openssl/config.yml
https://github.com/conan-io/conan-center-index/blob/master/recipes/openssl/1.x.x/conandata.yml

This is also the reason new OpenSSL pull requests fail for now: #2256

But adding all those versions in all configurations would mean much more load on the CI for no too good reason. It is also a terrible experience for contributors who might need to wait for hours until they get an error message for some configuration they can't test on their own machine.

Removing older versions from conandata.yml means that people who are still relying on those versions can't use the recipe unmodified anymore, which would be annoying too.

Minimum solution

I believe that we need sooner or later a new solution to prevent all versions from building for all configurations without removing the information about them from the conandata.yml file.

Since this is CI specific, I propose some new attribute in the config.yml which makes disabling builds explicit, so it also can't happen on accident.

Could look something like this:

versions:
  1.1.1d:
    folder: "1.x.x"
    build: false
  1.1.1e:
    folder: "1.x.x"
  1.1.1f:
    folder: "1.x.x"
  1.1.1g:
    folder: "1.x.x"

If there is no build attribute it defaults to true.

This would require changes

  • to the build bot to acknowledge such an attribute and skipping those builds
  • and a change to hook 052 to acknowledge such an attribute and not raise an error

Advanced solution

The above proposal would solve a too huge and often unnecessary load on the C3I, while still allowing to build all versions for a recipe locally when using the recipes from the git repository directly, which many do anyways.

However, this also means that consumers won't get further recipe modifications (including fixes) from the Conan Center remote for those older versions. Take #2256 once again as an example (we don't build OpenSSL < 1.0.2s and for the reasons above we probably don't want too; it would add ~ 19*110 builds in an instance).

To get recipe modifications from the Conan Center remote too, we could add an attribute which causes C3I to upload the sources-only without any built packages.

This could look like this:

versions:
  1.1.1d:
    folder: "1.x.x"
    build: "source_upload_only"
  1.1.1e:
    folder: "1.x.x"
  1.1.1f:
    folder: "1.x.x"
  1.1.1g:
    folder: "1.x.x"

This would require changes

  • to the build bot to acknowledge such an attribute, skipping any builds and upload only the recipe as source
  • and a change to hook 052 to acknowledge such an attribute and not raise an error

If we want to support both: No uploads at all and recipe source-only uploads, we could also support both false and source_upload_only values for such an attribute.

@Croydon Croydon mentioned this issue Sep 3, 2020
4 tasks
@madebr
Copy link
Contributor

madebr commented Sep 3, 2020

I like the advanced solution.
It can be used to tag the support status of a recipe/package version and give a hint to the ci how thoroughly it needs to test.
This can also be used by the CI to check whether a recipe uses deprecated dependencies.

@Croydon
Copy link
Contributor Author

Croydon commented Sep 7, 2020

Boost is another example where this is already hurting us. I think we need at the minimum a fast workaround, even if this means disabling the hook until we have an actual solution

#2842 (comment)

@jgsogo jgsogo added infrastructure Waiting on tools or services belonging to the infra priority: high labels Sep 17, 2020
@jgsogo
Copy link
Contributor

jgsogo commented Sep 17, 2020

Thanks for writing down this problem. I totally agree that we need a mechanism to stop building old versions (boost in #2936 took 10 hours!), but at the same time we need to keep building them to know if the recipe still works for those versions... we cannot upload to ConanCenter a recipe for a reference without running minimal testing on it, otherwise we can get a lot of bug reports about errors while building these old references (this is my argument against the source_upload_only alternative).

My proposal would be to compute less profile combinations for old versions, following your proposal we can assign different profile combinations to different versions: production (default), maintenance,...

versions:
  1.1.1d:
    folder: "1.x.x"
    profiles: "maintenance"
  1.1.1e:
    folder: "1.x.x"
  1.1.1f:
    folder: "1.x.x"
  1.1.1g:
    folder: "1.x.x"

Challenges here:

  • Define the minimum set of profiles for maintenance: right now we have different sets of profiles, production, staging and develop and we use one file or the other depending on the environment (we have replicas of cci to test), so this is easy to implement.
  • Implement/improve the diff when a PR arrives
  • Define who can modify this file (I'd say all the community reviewers): we'd want to avoid a user removing all the maintenance from all the versions and submitting several PRs that will block the CI.

PS.- I would suggest a similar approach to declare the options to explode (we already need it to generate some packages for consumers that require an option with a value other than the default), again it is important to limit these changes to selected users.

@madebr
Copy link
Contributor

madebr commented Oct 10, 2020

This feature can be used to limit the number of combinations that c3i currently builds for tools packages such as cmake, cc65, ...
(complete list at #3157 (comment))

The pr at #3153 will only handicap the recipe because we hide information that might be useful for conan users. Fast switching between Release/Debug for debugging a tool becomes impossible. Never underestimate power users.

I solo brain-stormed a bit and came up with a following example config.yml.
It's based on @jgsogo's proposal.

If more complex conditions are required, then I would put the code in python scripts.
Mixing code and yml is imho bad practice.

sources:
  "1.25":  # build all combinations (c3i.v1.default_policy="all", c3i.v1.policies.all)
    folder: "all"
  "1.24":  # Build all combinations (c3i.v1.policies.all))
    folder: "all"
    profiles: "all"
  "1.23":  # Will only build tool (c3i.v1.policies.tool)
    folder: "all"
    profiles: "windows"
  "1.22":  # Build only for Windows (c3i.v1.policies.windows)
    folder: "all"
    profiles: "tool"
  "1.21":  # Will build extra options
    folder: "all"
    profiles: "extra_options"
  "1.20":  # Will build only on windows with extra options(binary and: common in policies)
    folder: "all"
    profiles: "extra_options & windows"
  "1.19":  # Will build on linux and windows (binary or: common)
    folder: "all"
    profiles: "linux + windows"
  "1.10":  # Ancient versions are in maintenance mode
    folder: "all"
    profiles: "maintenance"

c3i:
  v1:
    default_policy: "all"  # implicitly defined to "all"
    policies:
      all: {} # implicitly defined
      maintenance: # implicitly defined
        filter:
        - "[settings.os, settings.compiler, settings.compiler.version] == ['Linux', 'gcc', 10]"
        - "[settings.os, settings.compiler, settings.compiler.version] == ['Linux', 'clang', 10]"
        - "[settings.os, settings.compiler, settings.compiler.version] == ['Windows', 'Visual Studio', 17]"
        - "[settings.os, settings.compiler, settings.compiler.version] == ['Macos', 'apple-clang', 12]"
      tool: # implicitly defined
        drop: "settings.compiler,settings.build_type"  # Remove settings.compiler, settings.build_type axes from matrix (c3i should order all jobs in order of priority and choose the one with highest priority)
      windows: # example policy to only build on Windows
        filter: "settings.os == 'Windows'"  # Only build packages for which the filter is True
      linux: # example policy to only build on Linux
        filter: "settings.os == 'Linux'"  # Only build packages for which the filter is True
      extra_options:
        add_options:
          - opt1a: "True"  # Add {"opt1a": True, "opt1b": True} packages
            opt1b: "True"
          - opt2: "False"
          - opt3: "all" # Build (True and Fails], or build  all in ("openssl", "wolfssl", "libressl", "gnutls")
          - opt4: "openssl" # Add {"opt4": "openssl"} packages
          - opt5: ["openssl", "wolfssl"]

By changing the policies in the c3i section, you can change what packages are built by c3i.
In a sense, it acts as a reduced-functionality conan-package-tools interface.

Challenges here:

* Define the minimum set of profiles for `maintenance`: right now we have different sets of profiles, `production`, `staging` and `develop` and we use one file or the other depending on the environment (we have replicas of cci to test), so this is easy to implement.

This is addressed in my example.
Versions in maintenance mode should build for less systems.
I have written the maintenance policy explicitly here, but it can also be implicitly defined.

* Implement/improve the diff when a PR arrives

I don't know how this touches my proposal.

* Define who can modify this file (I'd say all the _community reviewers_): we'd want to avoid a user removing all the `maintenance` from all the versions and submitting several PRs that will _block_ the CI.

For my proposal, if a bot detects that the number of builds increases with a certain amount,
then it can refuse to build or ignore the change until the change is approved by reviewers.

PS.- I would suggest a similar approach to declare the options to explode (we already need it to generate some packages for consumers that require an option with a value other than the default), again it is important to limit these changes to selected users.

My proposal addresses this as well by adding extra options

This proposal is overkill, but some ideas might be useful. I'd love to hear your feedback.

@jgsogo
Copy link
Contributor

jgsogo commented Apr 8, 2021

This came up in one of the latest meetings with the team. Together with the number of configurations to build it is one of the issues that will bite us in the long term.

But, in this meeting, more people agree that the way to go is to remove versions from this repository (recipes and packages will always be available in the Conan remote, of course). The reason is that building less configurations is not a way to solve the problem and can introduce other errors:

  • even if we build less configurations, in the long-term, we will face the problem again. It is not a solution.
  • if we stop to build some references... then those refs are not being tested, it is like having commented/dead code in the repository.

Looks like the better approach is to remove the versions. It must be a bold decision, not just something that will postpone the problem rather than solve it. Some rules to follow:

  • Only, if it starts to be a problem in terms of building time (or random errors that are more likely when there are more versions).
  • Keep the latest patch for each minor version (of course it depends on the versioning schema used by the library)
  • Do our best, work on a case-by-case basis (maybe there are reasons to keep building a specific patch version).

Wdyt? is there any other way to make it sustainable in the long term?

This was referenced Jun 4, 2021
ericLemanissier added a commit to bincrafters/conan-center-index that referenced this issue Jun 8, 2021
ericLemanissier added a commit to bincrafters/conan-center-index that referenced this issue Jun 8, 2021
ericLemanissier added a commit to bincrafters/conan-center-index that referenced this issue Jun 9, 2021
madebr pushed a commit to madebr/conan-center-index that referenced this issue Jun 21, 2021
@Croydon
Copy link
Contributor Author

Croydon commented Jun 20, 2024

But, in this meeting, more people agree that the way to go is to remove versions from this repository (recipes and packages will always be available in the Conan remote, of course).

This proposal was denied by the team, which is fine. Some arguments against it are reasonable.

What I found unpleasant is that

if we stop to build some references... then those refs are not being tested, it is like having commented/dead code in the repository.

was completely broken as a rule by #16503 (and it also completely contradicts years-long efforts to build from source), but that is somewhat off-topic.

@Croydon Croydon closed this as not planned Won't fix, can't repro, duplicate, stale Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Waiting on tools or services belonging to the infra priority: high
Projects
None yet
Development

No branches or pull requests

3 participants