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

Refactor real_projects tests to support Windows #406

Closed
jherland opened this issue Jan 3, 2024 · 0 comments · Fixed by #412
Closed

Refactor real_projects tests to support Windows #406

jherland opened this issue Jan 3, 2024 · 0 comments · Fixed by #412
Assignees

Comments

@jherland
Copy link
Member

jherland commented Jan 3, 2024

From a discussion on #397:

Since we're adding a section for Windows here that returns on its own, I guess we can leave the POSIX arm below unchanged? In other words, make the whole function this?:

    def venv_script_lines(self, venv_path: Path) -> List[str]:
        if sys.platform.startswith("win"):  # Windows
            python_path = venv_path / "Scripts" / "python.exe"
            return (
                [
                    f"rd /s /q {venv_path}",
                    f"{sys.executable} -m venv {venv_path}",
                    f"{python_path} -m pip install --upgrade pip",
                ]
                + [
                    f'{python_path} -m pip install --no-deps "{req}"'
                    for req in self.requirements
                ]
                + [
                    f"type nul > {venv_path / '.installed'}",
                ]
            )
        else:  # Assume POSIX
            return (
                [
                    f"rm -rf {venv_path}",
                    f"python3 -m venv {venv_path}",
                    f"{venv_path}/bin/pip install --upgrade pip",
                ]
                + [
                    f"{venv_path}/bin/pip install --no-deps {shlex.quote(req)}"
                    for req in self.requirements
                ]
                + [
                    f"touch {venv_path}/.installed",
                ]
            )

But as soon as I suggest that, I'm starting to see some more cracks forming here:

Taking a step back and examining what this function is actually used for: This is not just returning an awkward shell script to be passed to subprocess.run(" && ".join(venv_script), ...) below. It is also used to create the string we return from venv_hash(), and this string should remain stable as long as the Python version, the venv_path (artificially fixed) and the script itself does not change. However, we're now introducing sys.executable into this script, and I don't know if this is always stable between successive CI runs (a CI runner could have the same Python installed in a different location).

Still, I'm not sure it's worth doing too much about this: The failure mode would be for the hash to update needlessly which would cause Windows tests to not benefit from the caching we're implementing here. Probably not too bad, all things considered.

To be clear: Since we're disabling real_projects on Windows for now, anyway, I'm happy for you to push all of these related suggestions to a later issue/PR. 😄


...because there are also some other minor things that I would like to change:

  • We should be running {venv_path}/bin/python -m pip install ... instead of {venv_path}/bin/pip install ... in the POSIX version as well.
  • We should be quoting paths in case they contain spaces (paths with spaces are certainly more common on Windows)
  • I also dislike the clumsy concatenation of lists above, and wonder if we could just yield instead...

Here's an attempt to rewrite this function into something that is hopefully more robust, while keeping the overall design (which arguably should be rewritten altogether, at some point):

    def venv_script_lines(
        self, venv_path: Path, python_exe: Optional[str] = None
    ) -> Iterator[str]:
        if python_exe is None:  # Default to current Python executable
            python_exe = sys.executable

        if sys.platform.startswith("win"):  # Windows
            remove_cmd = "rd /s /q"
            touch_file_cmd = "type nul >"
            venv_python = venv_path / "Scripts" / "python.exe"
        else:  # Assume POSIX
            remove_cmd = "rm -rf"
            touch_file_cmd = f"touch"
            venv_python = venv_path / "bin" / "python"

        def sh_quote(arg: Union[Path, str]) -> str:
            if sys.platform.startswith("win"):  # Windows
                return f'"{arg}"'
            else:  # Assume POSIX
                return shlex.quote(str(arg))

        yield f"{remove_cmd} {sh_quote(venv_path)}"
        yield f"{sh_quote(python_exe)} -m venv {sh_quote(venv_path)}"
        yield f"{sh_quote(venv_python)} -m pip install --upgrade pip"
        for req in self.requirements:
            yield f"{sh_quote(venv_python)} -m pip install --no-deps {sh_quote(req)}"
        yield f"{touch_file_cmd} {sh_quote(venv_path / '.installed')}"

Originally posted by @jherland in #397 (comment)

@jherland jherland self-assigned this Jan 3, 2024
jherland added a commit that referenced this issue Jan 20, 2024
This fixes issue #406.

Instead of generating lines of a shell script that need to be run with
shell=True in subprocess.run(), generate a sequence of argv-style
commands. These are safer (no shell involved) and more easily portable
to Windows (for one, our quoting of command line args on Windows was
dubious, at best...).

Also move the first and last commands in the previous shell script
(i.e. the preliminary removal of venv_dir, and the creation of the
venv_dir/.installed marker file) out of venv_script_lines() and into
the __call__() method.

Since these are no longer part of the generated commands, they are also
no longer part of the venv_hash(). If we make changes to these two steps
(without making any other changes that are captured by venv_hash(), we
will end up reusing a cache entry. If this is problematic, then some
other modification to change the venv_hash() must be added at the same
time.
@jherland jherland linked a pull request Jan 20, 2024 that will close this issue
jherland added a commit that referenced this issue Jan 25, 2024
This fixes issue #406.

Instead of generating lines of a shell script that need to be run with
shell=True in subprocess.run(), generate a sequence of argv-style
commands. These are safer (no shell involved) and more easily portable
to Windows (for one, our quoting of command line args on Windows was
dubious, at best...).

Also move the first and last commands in the previous shell script
(i.e. the preliminary removal of venv_dir, and the creation of the
venv_dir/.installed marker file) out of venv_script_lines() and into
the __call__() method.

Since these are no longer part of the generated commands, they are also
no longer part of the venv_hash(). If we make changes to these two steps
(without making any other changes that are captured by venv_hash(), we
will end up reusing a cache entry. If this is problematic, then some
other modification to change the venv_hash() must be added at the same
time.
jherland added a commit that referenced this issue Feb 29, 2024
This fixes issue #406.

Instead of generating lines of a shell script that need to be run with
shell=True in subprocess.run(), generate a sequence of argv-style
commands. These are safer (no shell involved) and more easily portable
to Windows (for one, our quoting of command line args on Windows was
dubious, at best...).

Also move the first and last commands in the previous shell script
(i.e. the preliminary removal of venv_dir, and the creation of the
venv_dir/.installed marker file) out of venv_script_lines() and into
the __call__() method.

Since these are no longer part of the generated commands, they are also
no longer part of the venv_hash(). If we make changes to these two steps
(without making any other changes that are captured by venv_hash(), we
will end up reusing a cache entry. If this is problematic, then some
other modification to change the venv_hash() must be added at the same
time.
jherland added a commit that referenced this issue Mar 7, 2024
This fixes issue #406.

Instead of generating lines of a shell script that need to be run with
shell=True in subprocess.run(), generate a sequence of argv-style
commands. These are safer (no shell involved) and more easily portable
to Windows (for one, our quoting of command line args on Windows was
dubious, at best...).

Also move the first and last commands in the previous shell script
(i.e. the preliminary removal of venv_dir, and the creation of the
venv_dir/.installed marker file) out of venv_script_lines() and into
the __call__() method.

Since these are no longer part of the generated commands, they are also
no longer part of the venv_hash(). If we make changes to these two steps
(without making any other changes that are captured by venv_hash(), we
will end up reusing a cache entry. If this is problematic, then some
other modification to change the venv_hash() must be added at the same
time.
jherland added a commit that referenced this issue Mar 11, 2024
This fixes issue #406.

Instead of generating lines of a shell script that need to be run with
shell=True in subprocess.run(), generate a sequence of argv-style
commands. These are safer (no shell involved) and more easily portable
to Windows (for one, our quoting of command line args on Windows was
dubious, at best...).

Also move the first and last commands in the previous shell script
(i.e. the preliminary removal of venv_dir, and the creation of the
venv_dir/.installed marker file) out of venv_script_lines() and into
the __call__() method.

Since these are no longer part of the generated commands, they are also
no longer part of the venv_hash(). If we make changes to these two steps
(without making any other changes that are captured by venv_hash(), we
will end up reusing a cache entry. If this is problematic, then some
other modification to change the venv_hash() must be added at the same
time.
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 a pull request may close this issue.

1 participant