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

Update infromation about pip_pre configuration options #2885

Closed
wants to merge 2 commits into from

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Jan 20, 2023

The docs for tox configuration still mention --pre flag even if tox uses PIP_PRE environment variables. This may lead to confusion, as in #2690.
In this PR, I updated documentation to allow understanding of this change without looking for closed issues.

@@ -726,8 +726,7 @@ Pip installer

Determines the command used for installing packages into the virtual environment; both the package under test and its
dependencies (defined with :ref:`deps`). Must contain the substitution key ``{packages}`` which will be replaced by
the package(s) to install. You should also accept ``{opts}`` -- it will contain index server options such as
``--pre`` (configured as ``pip_pre``).
the package(s) to install. You should also accept ``{opts}`` -- it will contain index server options.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As pre-release is configured now with the environment variable, I removed it from the example but do not have any idea what could be the replacement.

As I understand, currently, all pip options could be configured using environment variables, so I'm not sure if opts is really required and maybe should be removed.

@Czaki
Copy link
Contributor Author

Czaki commented Jan 20, 2023

I cannot find information if pip 20.3.4 (last supporting python 2.7) supports configuration via environment variables.

@@ -751,9 +750,10 @@ Pip installer
:default: false
:version_added: 1.9

If ``true``, adds ``--pre`` to the ``opts`` passed to :ref:`install_command`. This will cause it to install the
If ``true``, adds ``PIP_PRE=1`` to the `environment <https://pip.pypa.io/en/stable/topics/configuration/#environment-variables>`_. This will cause pip it to install the
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct.

Setting pip_pre = true in a [testenv] does cause tox to add --pre to the opts:

def post_process_install_command(self, cmd: Command) -> Command:
install_command = cmd.args
pip_pre: bool = self._env.conf["pip_pre"]
try:
opts_at = install_command.index("{opts}")
except ValueError:
if pip_pre:
install_command.append("--pre")
else:
if pip_pre:
install_command[opts_at] = "--pre"
else:
install_command.pop(opts_at)
return cmd

The environment variable PIP_PRE is only set when using the legacy CLI flag --pre:

our.add_argument(
"--pre",
action="store_true",
help="deprecated use PIP_PRE in set_env instead - install pre-releases and development versions of"
"dependencies; this will set PIP_PRE=1 environment variable",
)

if getattr(option, "pre", False):
set_env["PIP_PRE"] = "1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The environment variable PIP_PRE is only set when using the legacy CLI flag --pre:

Is there a plan to remove inconsistency? Or should I update the description in this PR to inform about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's my understanding that the "pip flags" accepted by the tox CLI are deprecated and will be eventually removed. The pip environment variables should be used to influence pip instead of passing (some accepted subset) of pip flags to tox.

Basically tox has no reason to maintain these pip flags now that pip exposes all of its options via environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.
But I still need to figure out what this means for this PR. Should it be closed? Modified (ex. add an explanation to avoid confusion)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Czaki we have the following doc for the cli flag already: https://tox.wiki/en/latest/cli_interface.html#tox-legacy---pre

So I don't think this patch is strictly needed. However if you wanted to mention the legacy CLI flag in the testenv config section as "this config value may be overridden externally via the --pre cli flag (deprecated) or by setting PIP_PRE=1". Also note that setting PIP_PRE=0 does NOT override the testenv config, since that will use the pip cli flag, which takes precedence over envvars.

Copy link
Member

Choose a reason for hiding this comment

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

If the user can set the pre flag, we should fix it so that it works. However, we should also document that this flag is deprecated and we encourage using the environment variables instead.

Copy link
Member

Choose a reason for hiding this comment

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

See here

docs/config.rst Show resolved Hide resolved
@gaborbernat
Copy link
Member

Seems this has stalled, closing for now we can reopen when you get time for it again.

@gaborbernat gaborbernat closed this Feb 5, 2023
@Czaki
Copy link
Contributor Author

Czaki commented Feb 5, 2023

Seems this has stalled, closing for now we can reopen when you get time for it again.

It is stalled because I'm confused what is proper solution.

Did best option is to describe difference between legacy --pre flag and pip_pre configuration option, or add code changes to uniform behavior (also marking pip_pre as deprecated as it could be configured using setenv configuration option?

@Czaki
Copy link
Contributor Author

Czaki commented Feb 8, 2023

As I read tox source code, it looks like it should be handled in the same way as setenv, some variant of EnvConfigSet that will return SetEnv instance with "PIP_PRE=1" as input text.

It is correct assumption?

If it us correct then where I should register for such a class? Or could you point proper part of the documentation where I could read?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants