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.

Sign up for GitHub

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

Jump to bottom

Implement site-wide configuration settings #1978

Merged
merged 1 commit into from
Aug 22, 2014
Merged

Implement site-wide configuration settings #1978

merged 1 commit into from
Aug 22, 2014

Conversation

r1chardj0n3s
Copy link
Member

  • add site_config_dirs() to appdirs to determine locations across OSes
  • add system_config_files to locations.py
  • add system_config_files to get_config_files() and re-order files entries to correct precedence
  • document changes to configuration files in user guide

Closes #309

dstufft
dstufft reviewed Aug 15, 2014
View reviewed changes
pip/appdirs.py
pathlist = []

# just in case that var isn't set but the user tries to use xdg anyway
xdg_path = os.path.join('/etc/xdg', appname)
Copy link
Member

Choose a reason for hiding this comment

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

Nit pick, couldn't this just be moved up top so that it says xdg_config_dirs = os.getenv('XDG_CONFIG_DIRS', '/etc/xdg')? I think that follows the spec better since it states that /etc/xdg is the default if XDG_CONFIG_DIRS is not set.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess just in the case that someone does have XDG_CONFIG_DIRS but doesn't have /etc/xdg in that. But nah, stuff 'em.

Copy link
Member

Choose a reason for hiding this comment

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

Well, if they are attempting to use the XDG directories but
didn't configure it explicitly. It doesn't cost anything to
check it by default and actually makes it easier to explain.

@dstufft
Copy link
Member

dstufft commented Aug 15, 2014

Couple comments but this otherwise looks really good to me.

@pfmoore pfmoore mentioned this pull request Aug 21, 2014
Closed
@r1chardj0n3s
Implement site-wide configuration settings
aedca3c 
* add site_config_dirs() to appdirs to determine locations across OSes
* add system_config_files to locations.py
* add system_config_files to get_config_files() and re-order files entries to correct precedence
* document changes to configuration files in user guide

Closes pypa#309
@r1chardj0n3s
Copy link
Member Author

Resubmitted with the Windows change in place.

@pfmoore
Copy link
Member

pfmoore commented Aug 22, 2014

👍 Looks good!

@Ivoz
Copy link
Contributor

Ivoz commented Aug 22, 2014

I (still) think we should do away with having different file extensions for the different OSes. Somewhat arbitrary, but I think Windows systems should also default to reading pip.conf. Of course if pip.ini already exists on a Windows system, read that instead. This way, especially in virtualenvs, you don't need to know what OS you're on in order to store the correct filename.

A use case would be someone storing a pip.conf in a repository designed to be checked out with a virtualenv active and telling pip to fetch from a custom index. With the current design, they'd need to do something finicky like storing a symlink or two identical files only differing by extension, to make sure the repository works from scratch on any OS.

@pfmoore
Copy link
Member

pfmoore commented Aug 22, 2014

I don't disagree, but for this PR (which only adds a machine-wide file) we should be consistent with what's there already. When we change the user-specific filename, we should change this one to match, but let's be consistent for now.

I know that means we need to consider migration in both places, but I don't think that's a big enough issue to delay this PR.

@dstufft
Copy link
Member

dstufft commented Aug 22, 2014

I agree with @pfmoore, let's keep this PR focused.

dstufft added a commit that referenced this pull request Aug 22, 2014
@dstufft
Merge pull request #1978 from r1chardj0n3s/system-config-file
8b505a3 
Implement site-wide configuration settings
@dstufft dstufft merged commit 8b505a3 into pypa:develop Aug 22, 2014
@jakul
Copy link

jakul commented Aug 22, 2014

Thank you @r1chardj0n3s ❤️

@dstufft dstufft mentioned this pull request Aug 30, 2014
Closed
@kpersohn kpersohn mentioned this pull request Mar 16, 2015
Closed
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@r1chardj0n3s @dstufft @pfmoore @Ivoz @jakul