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

Correctly report errors thrown by CommandLinePathFactory#create. #16022

Closed
wants to merge 1 commit into from

Conversation

tjgq
Copy link
Contributor

@tjgq tjgq commented Aug 2, 2022

This method is called while initializing the remote module [1]. Any exceptions
not derived from java.io.IOException cause a silent server crash.

[1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java;l=436;drc=f3211f00ae08746b5794ab01d404c32b43146aba

@tjgq tjgq requested a review from coeuvre August 2, 2022 13:33
@tjgq
Copy link
Contributor Author

tjgq commented Aug 2, 2022

@bazel-io fork 5.3.0

@tjgq
Copy link
Contributor Author

tjgq commented Aug 2, 2022

@Yannic

@@ -99,6 +100,9 @@ public void createWithNamedRoot() throws Exception {
.isEqualTo(filesystem.getPath("/path/to/output/base/foo"));
assertThat(factory.create(ImmutableMap.of(), "%output_base%/foo/bar"))
.isEqualTo(filesystem.getPath("/path/to/output/base/foo/bar"));

assertThat(factory.create(ImmutableMap.of(), "%workspace%//foo//bar"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why allow this?

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 seems gratuitous to disallow double-slashes when most other command-line utilities handle them correctly; in addition, we get it for free through PathFragment.create. We could even avoid the edge case by converting the whole string to a PathFragment before regex-matching the first segment, but I wanted to do the bare minimum to avoid complicating the cherry pick.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

@Yannic
Copy link
Contributor

Yannic commented Aug 2, 2022

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 2, 2022
@sgowroji
Copy link
Member

sgowroji commented Aug 2, 2022

@bazel-io fork 5.3.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 2, 2022
@sgowroji sgowroji added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Aug 3, 2022
@copybara-service copybara-service bot closed this in 11368be Aug 3, 2022
sgowroji pushed a commit to sgowroji/bazel that referenced this pull request Aug 3, 2022
This method is called while initializing the remote module [1]. Any exceptions
not derived from java.io.IOException cause a silent server crash.

[1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java;l=436;drc=f3211f00ae08746b5794ab01d404c32b43146aba

Closes bazelbuild#16022.

PiperOrigin-RevId: 464997904
Change-Id: I87fd5eaeb562d849bb830d68f1bfbbf06f6e0ea9
sgowroji added a commit that referenced this pull request Aug 3, 2022
)

This method is called while initializing the remote module [1]. Any exceptions
not derived from java.io.IOException cause a silent server crash.

[1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java;l=436;drc=f3211f00ae08746b5794ab01d404c32b43146aba

Closes #16022.

PiperOrigin-RevId: 464997904
Change-Id: I87fd5eaeb562d849bb830d68f1bfbbf06f6e0ea9

Co-authored-by: Tiago Quelhas <tjgq@google.com>
ShreeM01 added a commit that referenced this pull request Aug 9, 2022
* Updated Codeowners file (#16032)

* Propagate the error message when a credential helper fails. (#16030)

Closes #16012.

PiperOrigin-RevId: 464732834
Change-Id: If51ce914098ff17ffe23fa4614947e7f4a5088dc

Co-authored-by: Tiago Quelhas <tjgq@google.com>

* Migrate legacy desugar wrapper to new rlocation() (#16025)

RELNOTES: Fix for desugaring failure on Bazel+Android+Windows build scenario.
PiperOrigin-RevId: 464654335
Change-Id: I845093b134dc68ae541603008fe7fee701c7451c

Co-authored-by: Gowroji Sunil <48122892+sgowroji@users.noreply.github.com>

* Correctly report errors thrown by CommandLinePathFactory#create. (#16036)

This method is called while initializing the remote module [1]. Any exceptions
not derived from java.io.IOException cause a silent server crash.

[1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java;l=436;drc=f3211f00ae08746b5794ab01d404c32b43146aba

Closes #16022.

PiperOrigin-RevId: 464997904
Change-Id: I87fd5eaeb562d849bb830d68f1bfbbf06f6e0ea9

Co-authored-by: Tiago Quelhas <tjgq@google.com>

* Fix an issue that `incompatible_remote_build_event_upload_respect_no_… (#16045)

* Fix an issue that `incompatible_remote_build_event_upload_respect_no_…

…cache` doesn't work with BwtB.

The root cause is we use `Path` as key to check which files are omitted, but it also compares the underlying filesystem. When BwtB is enabled, the filesystem for the file is different so we failed to mark the file as omitted.

This change fixes that by using `PathFragment` as key.

Fixes #15527.

Closes #16023.

PiperOrigin-RevId: 465284879
Change-Id: I49bbd7a6055e0f234b4ac86a224886bd85797f71

* Update ByteStreamBuildEventArtifactUploader changes

Removing the import as it is throwing the error

Co-authored-by: Chi Wang <chiwang@google.com>

Co-authored-by: Tiago Quelhas <tjgq@google.com>
Co-authored-by: Ted <tedx@google.com>
Co-authored-by: Gowroji Sunil <48122892+sgowroji@users.noreply.github.com>
Co-authored-by: Chi Wang <chiwang@google.com>
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
@tjgq tjgq deleted the credhelper-err branch December 8, 2022 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants