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

Pip.conf settings are not parsed or respected #180

Open
calizarr opened this issue Aug 2, 2024 · 8 comments · May be fixed by #181
Open

Pip.conf settings are not parsed or respected #180

calizarr opened this issue Aug 2, 2024 · 8 comments · May be fixed by #181

Comments

@calizarr
Copy link

calizarr commented Aug 2, 2024

An example pip.conf with sensitive variables changed to look like BASH variables:

[global]
extra-index-url = https://aws:${CODEARTIFACT_AUTH_TOKEN}@${DOMAIN}-${ACCOUNT_ID}.d.codeartifact.us-east-1.amazonaws.com/pypi/public-pypi/simple/
index-url = https://aws:${CODEARTIFACT_AUTH_TOKEN}@${DOMAIN}-${ACCOUNT_ID}.d.codeartifact.us-east-1.amazonaws.com/pypi/private-releases/simple/

These are actually set using the pip config set global.extra-index-url so they are presumably following the correct convention.

The file is located at $HOME/.config/pip/pip.conf following the XDG_CONFIG_HOME even when it is located in the legacy location $HOME/.pip/pip.conf it is not respected either.

The pip documentation in general.

EDIT: It also does not respect the relevant environment variables either

@calizarr calizarr linked a pull request Aug 2, 2024 that will close this issue
@pombredanne
Copy link
Member

After a chat with with @calizarr here is the suggestion of a way to go:

I want neither #170 from @heliocastro that requires installing pip nor #181 from @calizarr that requires installaing pip and further spawns a subprocess

Instead the approach could be:

  • add environment variable to the command line such that it is easy to configure index URLs without changing how python-inspector is called (which helps ORT), using https://click.palletsprojects.com/en/8.1.x/arguments/#environment-variables . Note that we only support --index-url for now and not --extra-index-url and it needs some testing to see how we would handle multiple URLs with a single environment variable. The var could be named "PYTHON_INSPECTOR_INDEX_URL" or the shorter "PYINSP_INDEX_URL" already used in the test env vars.

  • we could use the pip.conf or pip.ini configuration files. For this we would need to: 1. add CLI options to either use the default locations from pip as detailed in https://pip.pypa.io/en/stable/topics/configuration/#location or use a specific pip.conf file as an option. we will need to be clear about precedence of the options and the environment variable above. To read the config files, we can use the approach of @heliocastro from Add support for global index-urls #170 but not using pip as a library. Instead we could vendor/extract the few files we need, ideally keeping keep history with git-filter-repo. The files would be:

@heliocastro
Copy link

HI guys

At this draft pr, I started to try to solve this one. Please feel free to go over it and tell me if it is somehow in the right direction.

I migrated initial global settings to a global pydantic_settings object, which allows to expects variables with PYTHON_INSPECTOR prefix. Example:
PYTHON_INSPECTOR_INDEX_URL = <something>

I choose PYTHON_INSPECTOR because the clarity.

This move allows to have a .env local file with the variables, have it as an environment variable, and or use the usual command line.

To match the current pip spec, I changed a little the behavior on how index_url was treated. By default, the pip.conf can contains only one single url, and was always overrides by the default one, others are summarily ignored.
So now, only the defined index_url is considered, if none is defined, pypi one is the default.

The additional EXTRA_INDEX_URLS and respective command is now the multiple option where any additional indexes could be added.

@pombredanne Little comment, i was able to pass the tests only with regen.

@heliocastro
Copy link

Btw, if this approach is ok, I will add the parsing from pip.conf to fit this model as the one of the resources in chain.
Would then be something like this order of priority overrides top to bottom:

  • pip.conf
  • index-url option on cli
  • .env file
  • environment variable

If none of the options are set, the pypi will always be the main.

@calizarr
Copy link
Author

I've made this PR to add environment variables using Click's built-in mechanism for the index-url flag.

#187

This doesn't solve everything but it's a relatively easy built-in start.

@heliocastro
Copy link

That approach works too, but does not tackle all the entries where index is used, and it will cause another extra point to look at in the entire codebase.
What i'm tried above, besides be more complex, is simply reduce the variable usage for not only the inspector indec, but for all code, in one single place.

@calizarr
Copy link
Author

calizarr commented Aug 13, 2024

Oh I understand, however, given that Click is being used for CLI commands, I think it's best to start there and then go from your draft PR.

I still think we need to have an option to use pip.conf and not use the PyPi simple index. My PR is just a start.

I don't see a reason to not take advantage of Click's features.

@heliocastro
Copy link

Agreed with that, but again, solving a single point, and again, what i want to avoid is exactly this, we solve one small place, then "later" we will continue.
"Later" never comes, someone else will have a similar issue in different place of code, different developer comes and solve in his way, and again, we end up with code that not easy to understand, not consistent.

What i want is try to solve it right, starting to cleanup the code, not just add another plaster over the code that will not be easy to understand for anyone beyond this post.

In other way, your idea of have option to ignore pip.conf is good, and i thinking in doing the same for .python-version files.

But exemplifying on how a global settings object make sense, if we set this disabling options in the settings object, it will be easily accessible through the code, and not need to be passing click contexts or even work, global imported variables always hard to track if something is not working.

@calizarr
Copy link
Author

calizarr commented Aug 15, 2024

We can certainly attempt to make sure that later is now.

One can still gather all the settings in one place and then pass them along. In this case, Click is still handling flags and env vars from the CLI. However, the settings object can still import those and then use them along the way. They are not mutually exclusive.

This isn't quite another plaster over the code rather than adding one extra feature of Click's built-in functionality. If reading the code for resolve_cli.py then an understanding of Click and/or reading their documentation would be the best way to understand how the CLI flags are working and by extension the envvars.

The ability to ignore configuration files (as it does now) is good and should remain the default as that is what is expected at this point. However, a flag/envvar to enable configuration file parsing in addition to what is in your PR makes total sense to me.

I attempted to keep my PR as simple as possible without rewriting as much code. In the case of the resolve_dependencies function, Click parses either the CLI flags (-/--) or envvars (PYINSP_INDEX_URL) and then passes them as arguments to that function, so there isn't a global variable hanging around. With a global settings object, instead of directly going to resolving dependencies, the function can then receive the arguments and place them in the global settings object and then go from there.

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.

3 participants