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

feat: Set parallelization level #130

Closed
wants to merge 1 commit into from
Closed

Conversation

LecrisUT
Copy link

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

@@ -26,6 +28,12 @@ export async function main(): Promise<void> {
const testPresetAdditionalArgs = actionLib.getInput(cmakeglobals.testPresetAdditionalArgs, false);
const packagePresetAdditionalArgs = actionLib.getInput(cmakeglobals.packagePresetAdditionalArgs, false);
const runVcpkgEnvFormatString = actionLib.getInput(vcpkgglobals.runVcpkgEnvFormatStringInput, false);

// Set parallelization level
const nproc = os.availableParallelism();
Copy link
Author

Choose a reason for hiding this comment

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

From what I've read os.availableParallelism should take into account the virtualization and get the actual available processors, i.e. nproc vs nproc --all

@lukka
Copy link
Owner

lukka commented Dec 21, 2023

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:

  • run-cmake is designed to minimize the number of inputs to be provided to the action, with the idea that all the build process must be defined in the CMakePresets.json to ensure a reproducible build process.
  • for the reason stated above, if this is not (adequately) controllable via CMakePresets.json, we should open a ticket on CMake to add such feature, rather the overcoming the tool limitation in the action.

Regarding the PR itself:

  • this is a change in behavior, and it must be opt-in. As of now, it changes the behavior and provides no user control.
  • you should be able to run npm install successfully, as long as you set up properly access to GitHub Packages.
  • As I commented on the related issue Use parallelization #129, we need to establish whether setting the environment variables is really needed, and why. 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, and it does not justify the addition more complexity to the action itself.

@LecrisUT
Copy link
Author

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
Copy link
Owner

lukka commented Jan 28, 2024

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

  • by searching on existing usages of CMAKE_BUILD_PARALLEL_LEVEL along with run-cmake, it looks like it suffices to set environment variable, which means adding a new input to run-cmake is not necessary.
  • is there any evidence that the user can set the right value of parallelization and improve the performance? In my experience, when using ninja generator, there is no need at all for the user to set anything, ninja will utilize all cores 100% CPU time maximizing performances.

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
Copy link
Owner

lukka commented Jul 5, 2024

Closing stale PR.

@lukka lukka closed this Jul 5, 2024
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.

2 participants