Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@godofredoc
Copy link
Contributor

Turns out subprocess.run is available only after 3.5 and it is failing in our bots.

Bug: flutter/flutter#81855

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Turns out subprocess.run is available only after 3.5 and it is failing
in our bots.

Bug: flutter/flutter#81855
@ricardoamador
Copy link
Contributor

Is this necessary if we are still waiting for Popen to finish? If I understand correctly the benefit of Popen would be that we could do other work asynchronously instead vs waiting for run to finish.

Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

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

LGTM

@godofredoc
Copy link
Contributor Author

g for Popen to finish? If I understand correctly the benefit of Popen would be that we could do other work asynchronously instead vs waiting for run to finish.

This command used to use check_call but I updated it to subprocess.run to be able to print stdout and stderr but it turns out that the python version used in the bots is <3.5.

In this case we are using popen only to be able to capture stderr and stdout in a way that is compatible with different versions of python.

Copy link
Contributor

@ricardoamador ricardoamador left a comment

Choose a reason for hiding this comment

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

LGTM

@godofredoc godofredoc added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 12, 2022
@auto-submit auto-submit bot merged commit a4a8f5e into flutter:main Sep 12, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2022
cfontas pushed a commit to cfontas/engine that referenced this pull request Sep 14, 2022
Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
@godofredoc godofredoc deleted the fix_run branch January 12, 2024 19:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants