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

Jump to bottom

Fix additional outputs in BEP when remote_download_minimal is used #15711

Closed
wants to merge 1 commit into from
Closed

Fix additional outputs in BEP when remote_download_minimal is used #15711

wants to merge 1 commit into from

Conversation

exoson
Copy link
Contributor

@exoson exoson commented Jun 21, 2022

Injects optional test outputs when using --remote_download_minimal and
uses the list of injected artifacts to list them in BEP TestResult.

@exoson exoson requested a review from a team as a code owner June 21, 2022 12:26
@exoson exoson mentioned this pull request Jun 21, 2022
Closed
@exoson
Copy link
Contributor Author

exoson commented Jun 21, 2022

Will be fixing the tests later. Would appreciate comments on if a change along these lines would make sense.

@sgowroji sgowroji added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-user-response Awaiting a response from the author labels Jun 22, 2022
@exoson
Fix additional outputs in BEP when remote_download_minimal is used
24e567e 
Injects optional test outputs when using --remote_download_minimal and
uses the list of injected artifacts to list them in BEP TestResult.
@coeuvre
Copy link
Member

coeuvre commented Jun 28, 2022

Thanks for your PR!

Since this PR is mainly about test strategy and is already beyond my limited understanding of it, I will try to find an appropriate reviewer.

Out of my head, are these additional outputs already referenced in BEP for normal build / remote build?

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jun 29, 2022
@exoson
Copy link
Contributor Author

exoson commented Jun 29, 2022

Out of my head, are these additional outputs already referenced in BEP for normal build / remote build?

Yes, the files are currently referenced as long as the files exist under bazel-testlogs. With --remote_download_minimal we don't download the test output files so they won't get referenced.

@exoson
Copy link
Contributor Author

exoson commented Jul 14, 2022

@coeuvre Any updates on finding a reviewer?

@coeuvre coeuvre mentioned this pull request Aug 18, 2022
Closed
@coeuvre
Copy link
Member

coeuvre commented Aug 18, 2022 •
edited
Loading

I went with another way (#16123) to fix this issue. It requires no changes outside remote module and allow us to remove the hack in StandaloneTestStrategy. That is also the way how we solve similar problems internally.

@exoson
Copy link
Contributor Author

exoson commented Aug 18, 2022

I went with another way (#16123) to fix this issue. It requires no changes outside remote module and allow us to remove the hack in StandaloneTestStrategy. That is also the way how we solve similar problems internally.

Thanks! I'll close this PR.

@exoson exoson closed this Aug 18, 2022
copybara-service bot pushed a commit that referenced this pull request Aug 22, 2022
@coeuvre @copybara-github
Save injected metadata in RemoteActionFileSystem
79b5b5d 
So that spawn outputs can be accessed among Spwans within the same action using the `FileSystem` API.

This allow us to revert the hack we introduced in #12590. Also fixes the issue described by #15711.

Closes #15711.

Closes #16123.

PiperOrigin-RevId: 469133936
Change-Id: Ide5bcfa0fe2c6a3806d333cd61270e411aa78f80
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
@coeuvre @aiuto
Save injected metadata in RemoteActionFileSystem
d81e58d 
So that spawn outputs can be accessed among Spwans within the same action using the `FileSystem` API.

This allow us to revert the hack we introduced in bazelbuild#12590. Also fixes the issue described by bazelbuild#15711.

Closes bazelbuild#15711.

Closes bazelbuild#16123.

PiperOrigin-RevId: 469133936
Change-Id: Ide5bcfa0fe2c6a3806d333cd61270e411aa78f80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@exoson @coeuvre @sgowroji @ShreeM01