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

support for POETRY_CONFIG_DIR, POETRY_DATA_DIR and POETRY_CACHE_DIR environment variables #5301

Merged
merged 1 commit into from
May 30, 2022

Conversation

mohan43u
Copy link
Contributor

  • use PROJECT_HOME if available instead of XDG variables in unix systems

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

I have created this new PR as I'm not at all satisfied modifying XDG variables in order to make poetry properly set its cache dir under my preferred directory. For some reason, my previous PR #3550 was rejected because I tried to modify depreciated get-poetry.py (which is silly because when that PR created, get-poetry.py was the primary way to install poetry) and I tried to use the already available POETRY_HOME variable to achieve my purpose.

Anyway, I rectified both issues and creating this PR in hope that poetry devs understand XDG variables are common variables used by many application and should be used as fallback rather then expecting users to modify XDG in order to make poetry store its cache under their preferred directory.

May fix #2445

@Secrus
Copy link
Member

Secrus commented May 20, 2022

This seems to be answering a very specific needs and is not really generic. PROJECT_HOME might be already used by someone in their setup and this change would break it.

@mohan43u
Copy link
Contributor Author

This seems to be answering a very specific needs and is not really generic.

the problem here is, there is no way to make poetry use a specific directory to keep all its stuff, if you look at pip, it can keep all its stuff under PYTHONUSERBASE and PIP_CACHE_DIR, pip dont use XDG_* variables (like poetry). This whole thing is all about providing way for poetry to keep its stuff under one place and not to depend on XDG_* variables.

PROJECT_HOME might be already used by someone in their setup and this change would break it.

yeah, PROJECT_HOME is wrong, POETRY_HOME is already wrong, If I change it to BLAHBLAHBLAHBLAH, it is also going to be used by somebody so it is also wrong. funny thing is, somebody said I should not use POETRY_HOME for anything, even poetry dont use POETRY_HOME apart from the installer. it looks like POETRY_HOME will be the datadir if it was set according to this line

poetry_home = os.getenv("POETRY_HOME")

Apologies for my rant. We can change this to something unique like POETRY_PROJECT_HOME, this is not going to be an issue.

All I'm asking is provide a way to keep poetry's files in one place just like how I can keep pip's files in one place by setting PYTHONUSERBASE and PIP_CACHE_DIR pointing to single directory.

@Secrus
Copy link
Member

Secrus commented May 21, 2022

Thank you for your input on the matter, I understand it better now. There is cache-dir setting in configuration. Wouldn't it be ok to use?

@mohan43u mohan43u changed the title added PROJECT_HOME support support for POETRY_CONFIG_DIR, POETRY_DATA_DIR and POETRY_CACHE_DIR environment variables May 23, 2022
@neersighted
Copy link
Member

neersighted commented May 23, 2022

I think this can be converted into a simple docs change -- the config mechanism supports environmental variables for all settings (and this is documented, though we don't call it out for this use case), so we have always supported POETRY_CACHE_DIR, POETRY_CONFIG_DIR, and POETRY_DATA_DIR -- try these out on your current install 😸

@mohan43u
Copy link
Contributor Author

I think this can be converted into a simple docs change -- the config mechanism supports environmental variables for all settings (and this is documented, though we don't call it out for this use case), so we have always supported POETRY_CACHE_DIR, POETRY_CONFIG_DIR, and POETRY_DATA_DIR -- try these out on your current install smile_cat

@mohan43u mohan43u closed this May 23, 2022
@neersighted
Copy link
Member

@mohan43u are you interested in rolling this PR over into documentation of the default paths/how to override them with env vars?

@mohan43u
Copy link
Contributor Author

mohan43u commented May 23, 2022

the config mechanism supports environmental variables for all settings (and this is documented, though we don't call it out for this use case), so we have always supported POETRY_CACHE_DIR, POETRY_CONFIG_DIR, and POETRY_DATA_DIR

https://python-poetry.org/docs/configuration/#using-environment-variables

I think the issue is outside of config module, the reason why #2445 happening is because of REPOSITORY_CACHE_DIR which is not handled by config module

So these environment variables are required outside of config.

@mohan43u mohan43u reopened this May 23, 2022
@mohan43u
Copy link
Contributor Author

mohan43u commented May 23, 2022

Wouldn't it be ok to use?

REPOSITORY_CACHE_DIRis outside of config module. only config module handles this setting https://python-poetry.org/docs/configuration/#cache-dir

@mohan43u
Copy link
Contributor Author

@mohan43u are you interested in rolling this PR over into documentation of the default paths/how to override them with env vars?

@neersighted we have to address REPOSITORY_CACHE_DIR issue. I think the code change which is available in this PR fixes this issue.

@neersighted
Copy link
Member

@mohan43u are you interested in rolling this PR over into documentation of the default paths/how to override them with env vars?

@neersighted we have to address REPOSITORY_CACHE_DIR issue. I think the code change which is available in this PR fixes this issue.

I'm not sure I understand the difference between REPOSITORY_CACHE_DIR and POETRY_CACHE_DIR. I can say that POETRY_CACHE_DIR is definitely respected -- try setting POETRY_CACHE_DIR=./cache and then doing poetry install on Poetry itself.

Can you please help me understand the specific expected behavior vs result, and how this change (which duplicates the existing env var logic in the config module) resolves it?

@mohan43u mohan43u closed this May 23, 2022
@mohan43u
Copy link
Contributor Author

I'm not sure I understand the difference between REPOSITORY_CACHE_DIR and POETRY_CACHE_DIR. I can say that POETRY_CACHE_DIR is definitely respected -- try setting POETRY_CACHE_DIR=./cache and then doing poetry install on Poetry itself.

I set POETRY_CACHE_DIR=~/testpoetry/cache, I still see ~/.cache/pypoetry/cache/repositories created by poetry which it should not suppose to do, this is the reason for #2445. This PR is basically modifying the behavior of poetry and make it more flexible in configuring default directories.

This is not at all relevant to the config environment variables which you are confusing with.

@mohan43u mohan43u reopened this May 23, 2022
@neersighted
Copy link
Member

neersighted commented May 23, 2022

Ah, I see, we're resolving/respecting the config early enough right now -- looks like the change that introduced REPOSITORY_CACHE_DIR forgot to think about config resolution order.

@abn came up with a fix for it offline, that fixes the issue by solving the underlying cause and has tests at #5672. I've merged that PR -- could you please test on the latest master and make sure it meets your use case?

@mohan43u
Copy link
Contributor Author

thanks for fixing this. I can now happily use POETRY_CACHE_DIR, POETRY_CONFIG_DIR (and most probably use POETRY_HOME) to achieve what I want.

@neersighted
Copy link
Member

thanks for fixing this. I can now happily use POETRY_CACHE_DIR, POETRY_CONFIG_DIR (and most probably use POETRY_HOME) to achieve what I want.

We could still use a PR that documents the default paths and how to override them with these variables explicitly if you're interested 😀

@mohan43u mohan43u reopened this May 24, 2022
@mohan43u
Copy link
Contributor Author

mohan43u commented May 24, 2022

We could still use a PR that documents the default paths and how to override them with these variables explicitly if you're interested grinning

Sure, I'll update the documentation. But I have doubt whether POETRY_DATA_DIR will work or not? as I see data_dir() definition inlocations.py uses POETRY_HOME environment variable if available, otherwise it is not using POETRY_DATA_DIR environment variable. It directly uses XDG location.

POETRY_CONFIG_DIR environment variable was properly used before falling back to XDG location in locations.py, but there is no use of POETRY_DATA_DIR environment variable.

@mohan43u mohan43u closed this May 24, 2022
@mohan43u
Copy link
Contributor Author

@neersighted I already updated this PR with the docs change. Let me know if any other changes we need to this PR.

@Secrus Secrus added the area/docs Documentation issues/improvements label May 25, 2022
@github-actions
Copy link

github-actions bot commented May 25, 2022

Deploy preview for website ready!

✅ Preview
https://website-ak2bhfoyb-python-poetry.vercel.app

Built with commit e5ffeb5.
This pull request is being automatically deployed with vercel-action

docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Gonna take a look in a bit, but I think we're definitely going to need a SPAG and formatting pass.

@mohan43u
Copy link
Contributor Author

@Secrus @neersighted pushed again with recommended changes.

@mohan43u
Copy link
Contributor Author

@neersighted @Secrus could you please check if anything required?

docs/configuration.md Outdated Show resolved Hide resolved
Secrus
Secrus previously approved these changes May 30, 2022
@Secrus Secrus requested a review from neersighted May 30, 2022 11:54
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

Needs a little refinement, and then all the commits should be squashed down to one. Looks good to merge after that.

docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
@mohan43u
Copy link
Contributor Author

Needs a little refinement, and then all the commits should be squashed down to one. Looks good to merge after that.

@Secrus @neersighted fixed as per the suggestions.

@neersighted neersighted merged commit c68011d into python-poetry:master May 30, 2022
@abn abn mentioned this pull request Jun 6, 2022
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/docs Documentation issues/improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

POETRY_CACHE_DIR or cache-dir config are not used at all.
3 participants