lukka / run-cmake

GitHub Action to build C++ applications with CMake (CMakePresets.json), Ninja and vcpkg on GitHub.
MIT License
181 stars 19 forks source link

feat: Set parallelization level #130

Closed LecrisUT closed 3 months ago

LecrisUT commented 10 months ago

I couldn't execute npm install so I have to rely on the CI to produce the dist-git

/usr/bin/npm install
npm ERR! code E401
npm ERR! 401 Unauthorized - GET https://npm.pkg.github.com/download/@lukka/run-vcpkg-lib/3.9.4/2ac5f82de33b196e62077d4f2218273d09b3b8a2 - authentication token not provided

npm ERR! A complete log of this run can be found in: /home/lecris/.npm/_logs/2023-11-29T12_31_49_400Z-debug-0.log

Question is, do you want it in here or in @lukka/run-cmake-lib? The only thing that is unclear is that the GH-Action variables would be in this repo, so how do you want to interface with the library to set it?

TODO:

lukka commented 9 months ago

I couldn't execute npm install so I have to rely on the CI to produce the dist-git

/usr/bin/npm install
npm ERR! code E401
npm ERR! 401 Unauthorized - GET https://npm.pkg.github.com/download/@lukka/run-vcpkg-lib/3.9.4/2ac5f82de33b196e62077d4f2218273d09b3b8a2 - authentication token not provided

npm ERR! A complete log of this run can be found in: /home/lecris/.npm/_logs/2023-11-29T12_31_49_400Z-debug-0.log

Question is, do you want it in here or in @lukka/run-cmake-lib? The only thing that is unclear is that the GH-Action variables would be in this repo, so how do you want to interface with the library to set it?

TODO:

  • [ ] Write test. Could use ProcessorCount to check what CMake detects it has, and then compare with the verbose call, to see what -J variable was set
  • [ ] Write GH-Action interface to overwrite the behavior

@LecrisUT thank you for contributing to this project! Here my thinkings:

Regarding the PR itself:

LecrisUT commented 9 months ago

By looking to the existing workflows on GH setting such variable, it looks like it suffices to set the environment variable in a env block

The issue is with supporting multiple os. Then it is not straightforward how to set it up, while in the JavaScript part, there is an abstraction layer to take care of that.

this is a change in behavior, and it must be opt-in. As of now, it changes the behavior and provides no user control.

Fair, it is on the todo list above. It needs discussion on what interface to provide. But at some point, shouldn't we also provide a useful default to always build and test in parallel in order to minimize the user's input?

The other more general issue regarding the presets I will comment in the issue

lukka commented 8 months ago

@LecrisUT here a summary of my thinking about this feature:

Let me know if my thinking is correct. I am really interested in seeing a scenario where the user have to set an explicit parallelism value in order to get a meaningful build time improvement.

lukka commented 3 months ago

Closing stale PR.