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

Fix Process output capture re: working_directory. #12197

Merged
merged 2 commits into from
Jun 12, 2021

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Jun 12, 2021

Previously our local Process output capturing did not match the REAPI
spec which led to local execution working and remote execution failing
whenever a Process execution used both working_directory and output
capturing.

Add a test that output capturing occurs relative to the
working_directory when set and fix our one combined use of
working_directory and output capturing in archive.py.

Fixes #12157

[ci skip-build-wheels]

Previously our local Process output capturing did not match the REAPI
spec which led to local execution working and remote execution failing
whenever a Process execution used both `working_directory` and output
capturing.

Add a test that output capturing occurs relative to the
`working_directory` when set and fix out one combined use of
`working_directory` and output capturing in `archive.py`.

Fixes pantsbuild#12157

[ci skip-build-wheels]
@jsirois
Copy link
Contributor Author

jsirois commented Jun 12, 2021

I tested this against remote-apis-testing by copying example-python into the pants repo and then editing remote-apis-testing with roughly:

$ git diff
diff --git a/docker-compose-templates/docker-compose-pants.jinja2_template b/docker-compose-templates/docker-compose-pants.jinja2_template
index aa4a682..831d56f 100644
--- a/docker-compose-templates/docker-compose-pants.jinja2_template
+++ b/docker-compose-templates/docker-compose-pants.jinja2_template
@@ -14,4 +14,4 @@ services:
       --remote-store-address=grpc://frontend:8980
       --remote-instance-name=remote-execution
       --remote-execution-extra-platform-properties=OSFamily=linux
-      test helloworld::
+      test example-python/helloworld::
diff --git a/docker-compose/docker/Dockerfile b/docker-compose/docker/Dockerfile
index 8a1c2e7..abda280 100644
--- a/docker-compose/docker/Dockerfile
+++ b/docker-compose/docker/Dockerfile
@@ -209,7 +209,9 @@ FROM build_env as pants_client
 # Note: Pants is installed in the source repository by a wrapper script and not on
 # any system path. Thus, there is no need to install Pants in this image.
 ARG PANTS_COMMIT
-RUN apt-get update && apt-get install -y curl git python3 python3-dev python3-distutils
-RUN git clone --branch=main https://github.com/pantsbuild/example-python.git
-WORKDIR /example-python
+RUN apt-get update && apt-get install -y curl git python3 python3-dev python3-distutils python3-venv
+RUN git clone --branch=main git://172.17.0.1/~jsirois/pantsbuild/jsirois-pants
+RUN curl https://sh.rustup.rs -sSf | bash -s -- -y
+ENV PATH="/root/.cargo/bin:${PATH}"
+WORKDIR /jsirois-pants
 RUN git checkout $PANTS_COMMIT
diff --git a/matrix.yml b/matrix.yml
index 84b0a64..10a4b38 100644
--- a/matrix.yml
+++ b/matrix.yml
@@ -82,11 +82,11 @@ projects:
     filename: docker-compose-pants.jinja2_template
     version_refs:
       PANTS_COMMIT:
-        value: 86b41de1e53658127cdd1b0798ae12aa0e2396c7
+        value: 20a007a707060bf82a473e7bbe896620be638c28
         update_function: get_latest_commit_hash_from_git_repo
         update_args:
-          repo: https://github.com/pantsbuild/example-python.git
-          ref: main
+          repo: git://172.17.0.1/~jsirois/pantsbuild/jsirois-pants
+          ref: issues/12157
   recc:
     filename: docker-compose-recc.jinja2_template
     version_refs:

Failed before this fix and worked after.

@@ -170,10 +170,10 @@ async def maybe_extract_archive(
digest: Digest, tar_binary: TarBinary, unzip_binary: UnzipBinary
) -> ExtractedArchive:
"""If digest contains a single archive file, extract it, otherwise return the input digest."""
output_dir = "__output"
extract_archive_dir = "__extract_archive_dir"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not gratuitous, but needed to avoid folks having to blow away their lmdb stores: for the same Process input this PR changes the output digest. I had to blow away my lmdb store to get tests passing but others should not have to take that drastic measure.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

Thanks for fixing.

This should probably be backported to 2.5.x?

@jsirois
Copy link
Contributor Author

jsirois commented Jun 12, 2021

This should probably be backported to 2.5.x?

Yeah. I'm really unclear on our maintenance policy. This bug stretches back to 1.30.x. I'll stick to just 2.5.x since I think we're pretty sure ~no-one is using remoting yet.

@jsirois jsirois merged commit 982de2a into pantsbuild:main Jun 12, 2021
@jsirois jsirois deleted the issues/12157 branch June 12, 2021 17:37
jsirois added a commit to jsirois/pants that referenced this pull request Jun 12, 2021
Previously our local Process output capturing did not match the REAPI
spec which led to local execution working and remote execution failing
whenever a Process execution used both `working_directory` and output
capturing.

Add a test that output capturing occurs relative to the
`working_directory` when set and fix out one combined use of
`working_directory` and output capturing in `archive.py`.

Fixes pantsbuild#12157

(cherry picked from commit 982de2a)

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
jsirois added a commit that referenced this pull request Jun 12, 2021
Previously our local Process output capturing did not match the REAPI
spec which led to local execution working and remote execution failing
whenever a Process execution used both `working_directory` and output
capturing.

Add a test that output capturing occurs relative to the
`working_directory` when set and fix out one combined use of
`working_directory` and output capturing in `archive.py`.

Fixes #12157

(cherry picked from commit 982de2a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

protoc fails in remote execution
3 participants