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
Use Fallback command on all platforms #3099
Conversation
@@ -824,11 +824,15 @@ def install_frontend_packages(packages: set[str], config: Config): | |||
# unsupported archs will use npm anyway. so we dont have to run npm twice | |||
fallback_command = ( | |||
get_package_manager() | |||
if constants.IS_WINDOWS and constants.IS_WINDOWS_BUN_SUPPORTED_MACHINE | |||
if not constants.IS_WINDOWS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the fallback always be npm
regardless of OS based on what we discussed? Why is there a case where it's still None ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For arm and 32bit machines, get_install_package_manager
will return with npm
as the primary command since bun
is not supported on these. Fallback needs to be None in these cases so npm
command is not ran twice
else None | ||
) | ||
processes.run_process_with_fallback( | ||
[get_install_package_manager(), "install", "--loglevel", "silly"], | ||
[get_install_package_manager(), "install"], # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#type: ignore
is a bit suspect here, but not blocking
The fallback command(
npm
) when installing packages should be used on all platforms.Also the
--loglevel
flag is used only for npm when installing packages. Bun already runs in debug mode by default and the--loglevel
flag has no effect