Skip to content

Fix additional outputs in BEP when remote_download_minimal is used#15711

Closed
exoson wants to merge 1 commit intobazelbuild:masterfrom
exoson:dev/additional_outputs
Closed

Fix additional outputs in BEP when remote_download_minimal is used#15711
exoson wants to merge 1 commit intobazelbuild:masterfrom
exoson:dev/additional_outputs

Conversation

@exoson
Copy link
Copy Markdown
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
Copy Markdown
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.
@exoson exoson force-pushed the dev/additional_outputs branch from 18d800c to 24e567e Compare June 28, 2022 11:37
@coeuvre
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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