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

refactor: Adapt to async version of spawn #648

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

pgrzesik
Copy link
Collaborator

Related to: #646 - sync use of spawn prevented interactive output to be updated

@pgrzesik pgrzesik self-assigned this Nov 24, 2021
try {
const pipTestRes = await spawn(pythonBin, ['-m', 'pip', 'help', 'install']);
return (
pipTestRes.stdoutBuffer &&
Copy link
Contributor

Choose a reason for hiding this comment

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

stdoutBuffer will be provided unconditionally, so no need to confirm on it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is that the same case with stderrBuffer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but in case of error handling, there could be an edge case, where the command fails because it cannot be invoked for some reason and then the error won't hold any of those properties

const pipTestRes = await spawn(pythonBin, ['-m', 'pip', 'help', 'install']);
return (
pipTestRes.stdoutBuffer &&
pipTestRes.stdoutBuffer.toString().indexOf('--system') >= 0
Copy link
Contributor

Choose a reason for hiding this comment

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

More natural would be includes('--system')

lib/docker.js Show resolved Hide resolved
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@pgrzesik pgrzesik merged commit 50c2850 into master Nov 24, 2021
@pgrzesik pgrzesik deleted the migrate-spawn-sync-to-async-version branch November 24, 2021 12:08
@obataku
Copy link

obataku commented Dec 2, 2021

@pgrzesik which version of nodejs supports Buffer.trim? not seeing it in 17.x

@pgrzesik
Copy link
Collaborator Author

pgrzesik commented Dec 2, 2021

Hello @obataku - I believe it's not supported at all on Buffer, are you observing some issues or such usage?

@obataku
Copy link

obataku commented Dec 2, 2021

@pgrzesik indeed, using dockerizePip with serverless-python-requirements@5.2.1 gives the following error:

  TypeError: ps.stdoutBuffer.trim is not a function
      at getDockerUid (.../node_modules/serverless-python-requirements/lib/docker.js:200:26)
      at async installRequirements (.../node_modules/serverless-python-requirements/lib/pip.js:341:30)
      at async installRequirementsIfNeeded (.../node_modules/serverless-python-requirements/lib/pip.js:682:3)
      at async ServerlessPythonRequirements.installAllRequirements (.../node_modules/serverless-python-requirements/lib/pip.js:760:29)

... which points to the following change introduced by this PR: https://github.com/serverless/serverless-python-requirements/blame/e3d9ebcdd3ec72cc2e3301d33e10dd10ef6a594d/lib/docker.js#L200

-  return ps.stdout.trim();
+  return ps.stdoutBuffer.trim();

@pgrzesik
Copy link
Collaborator Author

pgrzesik commented Dec 2, 2021

Thanks for reporting, I see the issue now where the stdout was string (surprisingly it could also be a Buffer but for some reason it worked). I will prepare a fix for it and issue a new release 👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants