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

Remove /opt/graphite prefix and use setuptools #835

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

piotr1212
Copy link
Member

This switches from distutils to setuptools and removes the default /opt/graphite prefix.

I think this should go in a major or at least a minor release (no patch release).

Reasons for removing /opt/graphite prefix:

  • Does not work with setuptools
  • Confuses new users/contributors
  • Is not common among Python applications
  • It is unclear how to disable it
  • Causes issues with PYTHONPATH (just google "No module named 'graphite'")
  • The code that accomplishes this in setup.py is ugly (rewriting setup.cfg)
  • Causes unexpected issues, e.g. Error when running pip install carbonate carbonate#101
  • There are better ways to install in other locations.
  • Makes installation of Graphite "harder", have to account for it in your Ansible playbooks/Chef recipes/Puppet Modules.
  • Requires root privileges on many hosts which don't have a /opt partition
  • Pisses me off every time I forget to set the GRAPHITE_NO_PREFIX environment variable.

I don't see any reasons for keeping it other than:

  • This has been the default for 10 years

If you need to install Graphite in an other location you should use:

  • virtualenv/conda (recommended)
  • --user switch
  • --user switch and a customized PYTHONUSERBASE
  • maybe even Docker

more on custom locations

Switch to setuptools gets rid of the following warnings:
UserWarning: Unknown distribution option: 'install_requires'
UserWarning: Unknown distribution option: 'long_description_content_type'

PPA discourages the use of distutils and suggests setuptools. Setuptools has some nice features we can use in future.

Don't install in /opt/graphite anymore, this is just strange and causes nothing
but trouble. Setuptools does not support it either.

Switch to setuptools to get rid of following warnings:
UserWarning: Unknown distribution option: 'install_requires'
UserWarning: Unknown distribution option: 'long_description_content_type'
@codecov-io
Copy link

codecov-io commented Jan 8, 2019

Codecov Report

Merging #835 into master will increase coverage by 0.32%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
+ Coverage   50.21%   50.54%   +0.32%     
==========================================
  Files          37       37              
  Lines        3541     3510      -31     
  Branches      508      508              
==========================================
- Hits         1778     1774       -4     
+ Misses       1646     1617      -29     
- Partials      117      119       +2     
Impacted Files Coverage Δ
bin/carbon-aggregator-cache.py 0.00% <ø> (ø)
bin/carbon-aggregator.py 0.00% <ø> (ø)
bin/carbon-cache.py 0.00% <ø> (ø)
bin/carbon-client.py 0.00% <0.00%> (ø)
bin/carbon-relay.py 0.00% <ø> (ø)
lib/carbon/conf.py 37.18% <0.00%> (-1.25%) ⬇️
lib/carbon/util.py 50.00% <100.00%> (+0.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42c1242...05b9ce9. Read the comment docs.

@piotr1212
Copy link
Member Author

We should probably also get rid of code snippets like these https://github.com/graphite-project/carbon/blob/master/bin/carbon-cache.py#L19-L26

@DanCech
Copy link
Member

DanCech commented Jan 8, 2019

I like this idea, the biggest hurdle for new users imho is how complicated it is to get set up so anything we can do to streamline things is great! We do need to make sure we handle upgrades properly and that the installation/upgrade docs are all clear and accurate.

@deniszh
Copy link
Member

deniszh commented Jan 8, 2019

Yep, that's a good question. We should provide nice doc to upgrade graphite which still installed in /opt/graphite to new setup.

@piotr1212
Copy link
Member Author

piotr1212 commented Jan 8, 2019 via email

Warning: 'classifiers' should be a list, got type 'tuple'
fallback to sys.prefix if GRAPHITE_ROOT is not set, instead of the directory above
the location where the binaries are installed.
@stale
Copy link

stale bot commented Apr 13, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 13, 2020
@ploxiln
Copy link
Contributor

ploxiln commented Apr 13, 2020

(copying over a comment from graphite-project/graphite-web#2409 (comment))

I had another thought about storage dirs ... I think the original idea behind using /opt/graphite is those storage and log dirs, which would be awkward in the python site-packages directory. Maybe the thing to do is divorce them from the graphite application root, and default to /opt/graphite/storage and /opt/graphite/log regardless of the install prefix? And not mention them in setup.py at all? I suppose the downside is they would not be created by install. Just an idea - I suppose I always customize these dirs anyway.

@stale stale bot removed the stale label Apr 13, 2020
@deniszh
Copy link
Member

deniszh commented Apr 13, 2020

That could be good solution, @ploxiln

@piotr1212
Copy link
Member Author

Yup, I don't think how I did it is the right way. Maybe instead of /opt/graphite use something more FHS compliant like /var/lib/graphite and /var/log/graphite.

setup.py Outdated Show resolved Hide resolved
@piotr1212
Copy link
Member Author

Reverted graphite_root to /opt/graphite. This change is about not installing the python program code in /opt/graphite, can still use /opt/graphite for config and storage.

@@ -40,6 +29,8 @@
from carbon.client import CarbonClientManager # noqa
from carbon import log, events # noqa

CONF_DIR = join(sys.prefix, 'conf')
Copy link
Contributor

Choose a reason for hiding this comment

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

another sys.prefix that should be /opt/graphite?

Copy link
Contributor

Choose a reason for hiding this comment

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

(actually, this CONF_DIR variable is now unused)

@lgtm-com
Copy link

lgtm-com bot commented Apr 25, 2020

This pull request introduces 1 alert when merging 402e115 into 42c1242 - view on LGTM.com

new alerts:

  • 1 for Unused import

@stale
Copy link

stale bot commented Jun 24, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 24, 2020
@deniszh
Copy link
Member

deniszh commented Jun 24, 2020

I think it's important, will pin it.

@stale stale bot removed the stale label Jun 24, 2020
@deniszh deniszh added the pinned label Jun 24, 2020
@piotr1212
Copy link
Member Author

I really want to have this done but don't see that I will have the time anytime soon.

@deniszh
Copy link
Member

deniszh commented Jun 24, 2020

Hi @piotr1212 ,

No worries, I assigned you because it's your PR, it's not an obligation for you to finish that.

@vincele
Copy link

vincele commented May 8, 2023

This look like a really good cleanup, what is missing to allow progress on the subject ?

The hardcoded /opt/graphite is a bit sad though, I'd prefer more classical /var/{lib,log}/graphite defaults.

@deniszh
Copy link
Member

deniszh commented May 9, 2023

Unfortunately, that 's incompatible change. Probably, still can be done, together with major release.

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

Successfully merging this pull request may close these issues.

None yet

6 participants