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

set jobs parameter in cmake build presets(#15419) #15422

Merged

Conversation

Hailios
Copy link
Contributor

@Hailios Hailios commented Jan 9, 2024

Changelog: Feature: CMakeToolchain defines jobs in generated CMakePresets.json buildPresets.
Docs: Omit

this should be seen as a proof of concept for the feature request to have conan generate CMakePresets.json with jobs parameter set.

closes #15419

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • [?] I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@CLAassistant
Copy link

CLAassistant commented Jan 9, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and low risk. A small unit/integration test (not really necessary a functional one) could be enough to move this forward (I have checked with the team, good to move it)

@@ -6,6 +6,7 @@
from conan.tools.cmake.layout import get_build_folder_custom_vars
from conan.tools.cmake.toolchain.blocks import GenericSystemBlock
from conan.tools.cmake.utils import is_multi_configuration
import conan.tools.build as build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better import directly build_jobs, we are not using scoping in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks.

Did you check my comment above?

Looks good and low risk. A small unit/integration test (not really necessary a functional one) could be enough to move this forward (I have checked with the team, good to move it)

Do you think you can add some small test, it seems in https://github.com/conan-io/conan/blob/develop2/conans/test/integration/toolchains/cmake/test_cmaketoolchain.py would be the best, some of the existing ones can help? If not let me know and I'll do it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be great if you could do it :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I contributed the test, and also moved the definition to the "common" preset, so it also applies to the testPresets. I understand that we also want to do it, in the same way it affects also the CMake.test() helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the test. very good choice of parallel jobs ;)

this should not be added to the testPresets. I do not have deep enough CMake knowledge to know why, but cmake cannot parse the same jobs parameter in the testPresets.

example invalid presets sections

    "buildPresets": [
        {
            "name": "conan-debug",
            "configurePreset": "conan-debug",
            "jobs": 16
        }
    ],
    "testPresets": [
        {
            "name": "conan-debug",
            "configurePreset": "conan-debug",
            "jobs": 16
        }
    ]

produces the following error

$ cmake --build --target all --preset conan-relwithdebinfo
CMake Error: Could not read presets from <path to root directory>:
Error: @36,21: Invalid extra field "jobs" in Preset
            "jobs": 16
                    ^

it builds in parallel with "jobs": 16 in only the buildPresets, and more importantly does not fail to read the presets.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, yeah, lets remove it!

I'll leave jobs=42 (the answer to everything ;) only for buildPresets

@memsharded memsharded added this to the 2.1 milestone Jan 9, 2024
@memsharded memsharded self-assigned this Jan 10, 2024
@memsharded memsharded merged commit 5c61955 into conan-io:develop2 Jan 10, 2024
2 checks passed
@memsharded
Copy link
Member

Merged, it will be in next 2.1, thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants