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.

Sign up for GitHub

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

Revert prev lint fix #1604

Merged
merged 7 commits into from
Jan 13, 2023
Merged

Revert prev lint fix #1604

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5359885
Revert prev lint fix
DonggeLiu Jan 9, 2023
94cc54c
Ignore pylint error
DonggeLiu Jan 9, 2023
e98c11a
Revert all similar lint fixes
DonggeLiu Jan 9, 2023
f002ffb
Remove redundant code
DonggeLiu Jan 11, 2023
2a5f3fa
Merge branch 'master' into fix_local_exp
DonggeLiu Jan 11, 2023
53714cd
Revert "Revert all similar lint fixes"
DonggeLiu Jan 12, 2023
8ab0fb7
Merge branch 'master' into fix_local_exp
DonggeLiu Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions common/gcloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,9 @@ def run_local_instance(startup_script: Optional[str] = None) -> bool:
"""Does the equivalent of "create_instance" for local experiments, runs
|startup_script| in the background."""
command = ['/bin/bash', startup_script]
with subprocess.Popen(command,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT):
return new_process.ProcessResult(0, '', False)
# pylint: disable=consider-using-with
subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this does anything to check if the command succeeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks the return status of the command in the next line.

Copy link
Contributor

@oliverchang oliverchang Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This seems like the same behaviour as before: https://github.com/google/fuzzbench/pull/1529/files#diff-f61c598f4891541e693644bbc9016e5841fcddc986ea967eb3e192586a8c57deL133

The previous return was a no-op that would've always returned True (side note: is this behaviour actually correct?)

    subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
    return new_process.ProcessResult(0, '', False).retcode == 0

@alan32liu would be good to make this more explicit in the PR description in the future to make it clearer to reviewers :)

return True


def create_instance_template(template_name, docker_image, env, project, zone):
Expand Down