Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Update run.bzl #1933

Merged
merged 4 commits into from
Oct 1, 2021
Merged

Update run.bzl #1933

merged 4 commits into from
Oct 1, 2021

Conversation

dieend
Copy link
Contributor

@dieend dieend commented Sep 29, 2021

What is the current behavior?

container_run_and_extract is intentionally non-hermetic, because docker isn't sandboxed and may read arbitrary files. It can't be correctly invalidated from the cache, because some of the dependencies cannot be declared.

What is the new behavior?

Rules now accept extra dependency

@google-cla
Copy link

google-cla bot commented Sep 29, 2021

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 with @googlebot 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.

@google-cla google-cla bot added the cla: no label Sep 29, 2021
@dieend
Copy link
Contributor Author

dieend commented Sep 29, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Sep 29, 2021
@alexeagle alexeagle merged commit 1bb136e into bazelbuild:master Oct 1, 2021
@sluongng
Copy link
Contributor

sluongng commented Oct 2, 2021

I am a bit confused with this PR: there is a clear lack of context in the PR description and the commit messages are also lacking in term of quality.

I would suggest at minimum linking PR with an issue stating the problem?

@alexeagle
Copy link
Collaborator

@sluongng I agree, this is one of those no-time-for-maintainership issues where the code change is pretty clear but everything else about the PR needs work. I can choose between merging it or teaching the contributor, but probably not both

@alexeagle
Copy link
Collaborator

sorry I was a bit defensive there. Thanks for pointing this out @sluongng - next time I won't be so permissive with merging.

@sluongng
Copy link
Contributor

sluongng commented Oct 5, 2021

@alexeagle all is good. I know it takes a lot of effort being a maintainer and I know many bazelbuild projects are still in an early stage, process may be unclear.

There will always be a tradeoff between maintainability and speed but I have no clue where to draw the line for these projects.

I usually have to read HEAD of 'master/main' branch of these projects to find whether they are suitable to update our internal tracking. When a PR comes with unclear context, it might takes a bit of extra time and effort to digest.

Thanks for keeping the project fresh 🤝

@alexeagle
Copy link
Collaborator

I updated the original comment to reflect my understanding of the issue, so at least anyone arriving here from release notes or commit history can understand it better.

@dieend
Copy link
Contributor Author

dieend commented Oct 6, 2021

Sorry for not providing enough context. I was planning to update the PR description after further testing internally and didn't expect it's merged pretty soon.

Thank you for improving the PR description 🙏

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

Successfully merging this pull request may close these issues.

3 participants