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: Zip source directory should read from sh_work_dir #560

Merged
merged 4 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions package.py
Original file line number Diff line number Diff line change
Expand Up @@ -916,27 +916,37 @@ def execute(self, build_plan, zip_stream, query):
# XXX: timestamp=0 - what actually do with it?
zs.write_dirs(rd, prefix=prefix, timestamp=0)
elif cmd == "sh":
path, script = action[1:]
p = subprocess.Popen(
script,
shell=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
cwd=path,
)
with tempfile.NamedTemporaryFile(mode="w+t", delete=True) as temp_file:
path, script = action[1:]
# NOTE: Execute `pwd` to determine the subprocess shell's working directory after having executed all other commands.
script = f"{script} && pwd >{temp_file.name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this will break for powershell users, isn't it?

Maybe add a note that the pwd executed is required to determine the working dir of the subprocess shell after having executed all other commands. I find it a bit brittle, but now I understand that was a supported use case before #534.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this will break for powershell users, isn't it?

Not a powershell user too 😅 but hopefully this means we do not need to support pwsh
image

p = subprocess.Popen(
script,
shell=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
cwd=path,
)

p.wait()
call_stdout, call_stderr = p.communicate()
exit_code = p.returncode
log.info("exit_code: %s", exit_code)
if exit_code != 0:
raise RuntimeError(
"Script did not run successfully, exit code {}: {} - {}".format(
exit_code,
call_stdout.decode("utf-8").strip(),
call_stderr.decode("utf-8").strip(),
p.wait()
temp_file.seek(0)

# NOTE: This var `sh_work_dir` is consumed in cmd == "zip" loop
ANGkeith marked this conversation as resolved.
Show resolved Hide resolved
sh_work_dir = temp_file.read().strip()

log.info("WD: %s", sh_work_dir)

call_stdout, call_stderr = p.communicate()
exit_code = p.returncode
log.info("exit_code: %s", exit_code)
if exit_code != 0:
raise RuntimeError(
"Script did not run successfully, exit code {}: {} - {}".format(
exit_code,
call_stdout.decode("utf-8").strip(),
call_stderr.decode("utf-8").strip(),
)
)
)
elif cmd == "set:filter":
patterns = action[1]
pf = ZipContentFilter(args=self._args)
Expand Down
46 changes: 46 additions & 0 deletions tests/test_zip_source.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import os
from unittest.mock import MagicMock, Mock

from package import BuildPlanManager


def test_zip_source_path_sh_work_dir():
zs = Mock()
zs.write_dirs = MagicMock()

bpm = BuildPlanManager(args=Mock())

bpm.execute(
build_plan=[
["sh", ".", "cd $(mktemp -d)\n echo pip install"],
["zip:embedded", ".", "./python"],
],
zip_stream=zs,
query=None,
)

zs.write_dirs.assert_called_once()

zip_source_path = zs.write_dirs.call_args_list[0][0][0]
assert zip_source_path != f"{os.getcwd()}"


def test_zip_source_path():
zs = Mock()
zs.write_dirs = MagicMock()

bpm = BuildPlanManager(args=Mock())

bpm.execute(
build_plan=[
["sh", ".", "echo pip install"],
["zip:embedded", ".", "./python"],
],
zip_stream=zs,
query=None,
)

zs.write_dirs.assert_called_once()

zip_source_path = zs.write_dirs.call_args_list[0][0][0]
assert zip_source_path == f"{os.getcwd()}"