Skip to content

fix: correct concurrency to align with documentation #2014

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

Merged
merged 5 commits into from
Mar 17, 2025

Conversation

andrecloutier
Copy link
Contributor

@andrecloutier andrecloutier commented Mar 11, 2025

Summary

Prior to this change:

  • Default concurrency was "1" or "" (depending if there was an error). The documentation suggests this should be the number of cores.
  • Setting "auto" concurrency would pass "auto" to install script. The documentation suggests this should be the number of cores.

Fixes: asdf-vm/asdf-nodejs#409

Other Information

@andrecloutier andrecloutier marked this pull request as ready for review March 11, 2025 23:54
@andrecloutier andrecloutier requested a review from a team as a code owner March 11, 2025 23:54
@andrecloutier andrecloutier changed the title fix: add support for auto concurrency fix: correct concurrency to align with documentation Mar 11, 2025
@Stratus3D Stratus3D merged commit 807ea38 into asdf-vm:master Mar 17, 2025
8 checks passed
@Stratus3D Stratus3D mentioned this pull request Mar 17, 2025
@Stratus3D
Copy link
Member

Thanks for the fix @andrecloutier and thanks for the added tests!

Got any thoughts on this feature @andrecloutier ? I've been on the fence for years on this feature. It requires some amount of plumbing and documentation. asdf core has to properly read the config and set the ASDF_CONCURRENCY env variable (as you've fixed here) and then plugins are expected to read this env variable and use it somehow to choose the optimal concurrency for their builds. I feel like it'd be less overhead for us and not much more hassle for plugins to remove this feature from asdf corre and leave it up to plugins. Plugins can easily check the number of cores available and apply their own formula to determine their optimal concurrency. Since asdf core already passes all environment variables to plugin callbacks it'd be easy for each plugin to read it's own env variable (say ASDF_NODEJS_CONCURRENCY) and use it if set. I've never set ASDF_CONCURRENCY, and I doubt many users do.

@andrecloutier
Copy link
Contributor Author

Thanks for the fix @andrecloutier and thanks for the added tests!

Got any thoughts on this feature @andrecloutier ? I've been on the fence for years on this feature. It requires some amount of plumbing and documentation. asdf core has to properly read the config and set the ASDF_CONCURRENCY env variable (as you've fixed here) and then plugins are expected to read this env variable and use it somehow to choose the optimal concurrency for their builds. I feel like it'd be less overhead for us and not much more hassle for plugins to remove this feature from asdf corre and leave it up to plugins. Plugins can easily check the number of cores available and apply their own formula to determine their optimal concurrency. Since asdf core already passes all environment variables to plugin callbacks it'd be easy for each plugin to read it's own env variable (say ASDF_NODEJS_CONCURRENCY) and use it if set. I've never set ASDF_CONCURRENCY, and I doubt many users do.

This feature to me reminds me of MAKEOPTS in Gentoo Linux where its useful to set compile settings globally at the OS/Package Manager level. In some cases, you may want to set it to the number of cores to just get the compile done as fast as possible, in some other cases, you want a more conservative number so that the computer is still usable. So I could see some utility here that folks may want to control how many resources compiling their dependencies will take. The code to support this isn't crazy so I don't feel too bad about keeping it. Especially as we're trying to stabilize the transition from the bash version to go. But, I also haven't been maintaining this project for years. ;)

@Stratus3D
Copy link
Member

Especially as we're trying to stabilize the transition from the bash version to go. But, I also haven't been maintaining this project for years. ;)

Yes good point. I've already been pushing the limits of what users will tolerate with the breaking changes introduced in v0.16.x. Probably best to continue supporting this feature. Thanks again for the fix @andrecloutier !

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