carbon
carbon copied to clipboard
graphite-project
Remove /opt/graphite prefix and use setuptools
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. https://github.com/graphite-project/carbonate/issues/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
rootprivileges on many hosts which don't have a /opt partition - Pisses me off every time I forget to set the
GRAPHITE_NO_PREFIXenvironment 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)
-
--userswitch -
--userswitch and a customizedPYTHONUSERBASE - maybe even Docker
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.
Codecov Report
Merging #835 into master will increase coverage by
0.32%. The diff coverage is25.00%.
@@ 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%) |
:arrow_down: |
| lib/carbon/util.py | 50.00% <100.00%> (+0.51%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 42c1242...05b9ce9. Read the comment docs.
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
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.
Yep, that's a good question. We should provide nice doc to upgrade graphite which still installed in /opt/graphite to new setup.
Good points, I can take care of the docs, but that can take a while, I'm a bit busy the next two weeks.
I'll also have to check all example config files for paths.
On Tue, 8 Jan 2019, 14:32 Denis Zhdanov <[email protected] wrote:
Yep, that's a good question. We should provide nice doc to upgrade graphite which still installed in /opt/graphite to new setup.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/graphite-project/carbon/pull/835#issuecomment-452299208, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxrWvfbY48obZ5gKxAMj2xzNoSIc7fUks5vBJ3mgaJpZM4Z1ZZb .
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[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
(copying over a comment from https://github.com/graphite-project/graphite-web/pull/2409#issuecomment-465734061)
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.
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.
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.
This pull request introduces 1 alert when merging 402e1159743447f180bba13817bdb1448e8f4db3 into 42c124251e7036bc01ab307c0ac1fb2016a092ab - view on LGTM.com
new alerts:
- 1 for Unused import
![lgtm-com[bot] avatar](https://avatars.githubusercontent.com/in/17324?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
I really want to have this done but don't see that I will have the time anytime soon.
Hi @piotr1212 ,
No worries, I assigned you because it's your PR, it's not an obligation for you to finish that.
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.
Unfortunately, that 's incompatible change. Probably, still can be done, together with major release.