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

Add no-remote-cache and no-remote-exec execution requirements #8710

Conversation

SrodriguezO
Copy link
Contributor

@SrodriguezO SrodriguezO commented Jun 24, 2019

This resolves #7932

It introduces two new execution requirements:

  • no-remote-exec: disallows remote execution without preventing remote caching
  • no-remote-cache: disallows remote caching without preventing remote execution or local caching

The current behavior of no-remote and no-cache remain unchanged.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@SrodriguezO
Copy link
Contributor Author

I'd like to add some tests around the new execution requirements before merging, but I figured I'd open the PR so it can start getting some feedback

@SrodriguezO
Copy link
Contributor Author

@ishikhman when you get a minute could you look over this? It's fairly short, and I'd love some feedback :)

@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from f0ac726 to 34bbd88 Compare June 27, 2019 19:34
@SrodriguezO
Copy link
Contributor Author

Finished adding tests and squashed commits

Copy link
Contributor

@ishikhman ishikhman left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this change!

I keep thinking whether it makes sense to add no-remote-cache which behaves exactly the same as no-cache.. other than that - just a few notes from my side (will have another look next week, sorry for the delay)

@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from b5bf143 to d37986d Compare July 11, 2019 15:52
@SrodriguezO
Copy link
Contributor Author

Force-push was squashing commits and rebasing on master

@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from 4732e96 to 9007b78 Compare July 11, 2019 16:02
@SrodriguezO
Copy link
Contributor Author

SrodriguezO commented Jul 11, 2019

Force push was squashing commits

@SrodriguezO
Copy link
Contributor Author

@ishikhman @buchgr @ola-rozenfeld I think this is ready for final review :)

Copy link
Contributor

@ishikhman ishikhman left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks a lot :)
just a few nit comments from me, otherwise LGTM

@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from f265107 to 25259e6 Compare July 12, 2019 18:25
@SrodriguezO
Copy link
Contributor Author

Force push was squashing commits

@SrodriguezO
Copy link
Contributor Author

@buchgr @ola-rozenfeld could you review when you get a minute?

Copy link
Contributor

@ishikhman ishikhman left a comment

Choose a reason for hiding this comment

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

Sorry, missed the one use case in the initial review:
what if we have combined cache - remote (http for now) and local (disk) - AND no-remote-cache tag. For now, bazel would still be using both caches, but should use only disk cache in such a case.
I think we missed this use case. Could you please add test cases and maybe code changes for that?

@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from bb5066c to a81d501 Compare July 17, 2019 16:39
@SrodriguezO
Copy link
Contributor Author

Force-push was squashing commits and rebasing on master

@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from fbe1542 to 1f17262 Compare July 19, 2019 22:28
@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from e88ed86 to a372c8d Compare July 19, 2019 22:36
@SrodriguezO
Copy link
Contributor Author

Force-push was squashing commits and rebasing on master

@jin jin added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Jul 22, 2019
@SrodriguezO SrodriguezO force-pushed the add-execution-requirements-for-remote-execution-and-caching-control branch from a372c8d to 15d7c6b Compare July 23, 2019 07:13
@SrodriguezO
Copy link
Contributor Author

Woops, I wasn't actually rebasing on master before because our fork was a bit out of date 😅

Synced our fork and rebased on master now.

@SrodriguezO
Copy link
Contributor Author

@ishikhman could you review again when you get a chance?

Do we also need @buchgr and/or @ola-rozenfeld to review before merging?

Copy link
Contributor

@ishikhman ishikhman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!
@SrodriguezO I'll try to merge it in the next days :)

FYI for the future PRs: please avoid force pushing, it makes changes a bit less obvious during code review ;) Everything will be merged as one commit in the end, with the message taken from the PR description.

@SrodriguezO
Copy link
Contributor Author

Ah okay. Sorry about that. I'll avoid force pushes in the future. Thanks for the review!

@SrodriguezO
Copy link
Contributor Author

SrodriguezO commented Jul 24, 2019

@ishikhman what should I do about rebasing though?

For instance, the branch has a conflict, but to fix it I have to either rebase on master (which would require a force-push) or merge master in (which makes the git history a bit messy). Which path would you suggest?

Edit: In this particular case I was able to fix the conflict without merging master or rebasing because it was a very simple one, but my question stands for the general case :)

@ishikhman
Copy link
Contributor

@ishikhman what should I do about rebasing though?

In this case I usually prefer rebasing to master, cause that won't produce additional commits like merge would do :)
Sorry, the phrasing of my initial advice was a bit confusing. What I meant is this: please avoid amends or any other changes to existing comments, because this makes it a bit harder to see what exactly was changed during the review. Rebasing to master is fine ;)

PS: merge is coming today.

@bazel-io bazel-io closed this in 8860c3e Jul 25, 2019
@buchgr
Copy link
Contributor

buchgr commented Jul 25, 2019

yay! great job both!

@SrodriguezO
Copy link
Contributor Author

Sorry, the phrasing of my initial advice was a bit confusing. What I meant is this: please avoid amends or any other changes to existing comments, because this makes it a bit harder to see what exactly was changed during the review. Rebasing to master is fine ;)

I see; thanks for the advice! And thanks for all the feedback on the PR :)

@borkaehw borkaehw deleted the add-execution-requirements-for-remote-execution-and-caching-control branch September 12, 2019 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local actions in a remote execution build should be cachable
5 participants