-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Matomo (module, package) updates #69342
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on!
8b65c45
to
9f956fa
Compare
9f956fa
to
abd437c
Compare
@Kiwi please post configuration you are testing with. |
abd437c
to
681452b
Compare
(Already has it from IRC but for anyone else that may want it) minimal example |
I'll try to have an in-depth look on Sunday. Ahead: Just properly referencing the option instead of the value seems to be a good solution for the |
nixos/modules/rename.nix
Outdated
@@ -134,6 +134,7 @@ with lib; | |||
(mkRenamedOptionModule [ "services" "piwik" "enable" ] [ "services" "matomo" "enable" ]) | |||
(mkRenamedOptionModule [ "services" "piwik" "webServerUser" ] [ "services" "matomo" "webServerUser" ]) | |||
(mkRenamedOptionModule [ "services" "piwik" "phpfpmProcessManagerConfig" ] [ "services" "matomo" "phpfpmProcessManagerConfig" ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity what happens if you have services.piwik.phpfpmProcessManagerConfig
set in your configuration.nix
with this branch? (ie. an option which has been renamed and then removed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not sure if the way I did that is the right way and I don't know this well enough to even know of what other possible approaches there might be. it seemed logical to me, first attempt that fixed the error, seemed to work, and I didn't explore it much after that.
To answer your question...
# with services.piwik.*
00:54:19 [kiwi@absurd-nixos nixpkgs]$ sudo nixos-rebuild -I nixpkgs=. build
building Nix...
building the system configuration...
trace: warning: The option definition `services.matomo.phpfpmProcessManagerConfig' in `/home/kiwi/nixpkgs/nixos/modules/rename.nix' no longer has any effect; please remove it.
Use services.phpfpm.pools.<name>.settings
trace: warning: The option `services.piwik.phpfpmProcessManagerConfig' defined in `/etc/nixos/modules/matomo.nix' has been renamed to `services.matomo.phpfpmProcessManagerConfig'.
00:54:34 [kiwi@absurd-nixos nixpkgs]$ sudo vim /etc/nixos/modules/matomo.nix
# changed to services.matomo.*
00:54:58 [kiwi@absurd-nixos nixpkgs]$ sudo nixos-rebuild -I nixpkgs=. build
building Nix...
building the system configuration...
trace: warning: The option definition `services.matomo.phpfpmProcessManagerConfig' in `/etc/nixos/modules/matomo.nix' no longer has any effect; please remove it.
Use services.phpfpm.pools.<name>.settings
00:55:06 [kiwi@absurd-nixos nixpkgs]$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the message seems confusing so let's modify the mkRenamedOptionModule
to something like
(mkRemovedOptionModule [ "services" "piwik" "phpfpmProcessManagerConfig" ] "Use services.phpfpm.pools.<name>.settings")
@Kiwi it looks like you have resolved your issues, correct? |
Yes, I believe so and then some. (Improved/corrected the documentation more than I had, which also avoids the phpSocket definition altogether) |
a837eb8
to
681452b
Compare
ac8494c
to
4ce6299
Compare
@Kiwi Thanks for fixing this! I suck at using github's search and opened duplicate PR ↑. It adds simple NixOS test. Would you be interested in adding it to your PR? Feel free to use it: mmilata@068c0b2. It's missing a maintainer - I'm OK with being one if you're not interested. |
ping @Kiwi |
Fixes the phpfpm deprecation warnings about listen and extraConfig by using fpm.socket and settings. Removes phpfpmProcessManagerConfig.
Add a beta version of matomo 3.12 that has recent bug fixes. They release these more frequently so it's a good option to have.
Apologies this took so long. I hope I've done everything right... If I didn't mess anything up: I have reverted the beta back to the last version that worked which was 3.12.0-b3. I have tried the latest versions... they're still broken. It's up to version 3.12.0-rc2 at this point. I'm not sure how to fix it, either. I've looked briefly but I and I think everyone else that's looked at it have been incredibly busy >.> and I'm not on good (or recent) terms with PHP... I think I have the removed modules lines how they're supposed to be?? I git cherry-picked the test above that @mmilata supplied. I rebased on to master as of a few minutes ago because there were conflicts with the old PR. I've been watching the bot go, as I type, and it looks like it's probably going to pass (2 to go, 10 passed) Aha! there it is. 12 (you saw nothing!) passed. I wish I could get it building the newest version which I expect will be out soon...but as it is now, the beta 3 I believe is still better than what we have currently, 3.11.0. I know it's at least fixed one annoying bug that was bothering me in 3.11.0, I don't know either way if b3 introduced any bugs, but so far it's been fine... and I can't go back to 3.11.0 (the beta updated the database) on my server so I'd really appreciate if this was merged. :) Anyway, if this PR is satisfactory to all involved for what it does?? I think it should be merged. After all it does fix multiple problems; known bugs in 3.11.0 if you choose to use the beta, which is now possible, as well as the phpfpm deprecation warnings being resolved. And it has a test...! :) After that we can worry about getting the next version to work in a separate PR that won't conflict with this one.... as soon as anyone that can fix it has time. and add that other test... |
@GrahamcOfBorg test matomo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port to the new phpfpm options is done well and the reviewer feedback was adressed. We should merge this now and do a backport to 19.09, at least for the new phpfpm options. Finding the cause for the issues with the upcoming matomo version will need more time for research.
@Kiwi volunteered to maintain beta versions, so I'm fine with having multiple versions in nixpkgs. This could also be a handy way to package the new stable versions which do not contain security fixes, i.e. for which we don't want to change the default in stable, but to give users the choice to upgrade if they want without relying on unstable.
@mmilata thanks a lot for the test, did not think of such a simple version as I was busy thinking about the impossibility of matomo automated installs, but even just checking the installer page adds a lot of value to see wether the stack generally works.
Since @florianjacob has approved there isn't too much to say past this point. If this PR suits him it should be good to go. The only remaining thing for this PR is we might consider testing the |
(already addressed on IRC but for anyone else watching) @aanderse Yes, that sounds good. I should have it taken care of either tonight or tomorrow. |
@GrahamcOfBorg test matomo.matomo matomo.matomo-beta |
Thanks everyone for your contributions on this. In a future PR it would be nice to see this module take advantage of the |
HAHAHAHHA What the... I'm floored! This couldn't be more perfect Let me make sure I understand this correctly... You're saying that this PR, the primary purpose of which was to fix deprecation warnings, took me so long to wrap up that something else was deprecated in the meantime? Ha! (and if that's not true, please let me remain ignorant, just this once) Lest us not forget the number of betas (all broken after b3) of 3.12.0 there were. And indeed, less than 24 hours ago, the official version 3.12.0 was released. Which we still can't use. and of course who can forget about the 2 subsequent duplicate PRs during this time. This is the best laugh I've had all week. This has gone far beyond being mostly absurd to entirely. I love it. It's amazing. <3 |
Thanks for your patience on this one. For future reference had this been 3 separate PRs they probably would have been much faster to merge. Great job everyone! 🎉 |
Boo. :( I reject your reality! Maybe you mean 12 days? Yes, surely it was deprecated 12 days ago, not months.
Yeah, most likely, and in the future I'll keep that in mind... this time it ended up being incredibly amusing and I wouldn't have had to look at how to do the tests if it were 3. Thanks for all of your help :D |
Thanks alot to everyone! I will try to investigate the issues with the new 3.12 release soon. For the record: I intentionally did not use |
Backport to 19.09: #73692. I've dropped the commit that adds |
Motivation for this change
I got tired of seeing red every time I nixos-rebuilt my server so I updated Matomo. While doing so I found a bug in the latest version that is fixed in the next release (3.12.0) (edit: ...and then the b4 ended up breaking it on NixoOS which stalled this PR for a while... and now ~1 month later and 3.12.0 was released 20 hrs ago :| ) which is currently at beta 3. The project releases beta builds frequently and release builds not so often... so I think it'll be good to be able to use them. Forewarning, you can't downgrade (at least not easily, I don't know how to and I didn't try too hard, I'm sure if you had backups you could...) though as it changes the database. But it's been working better for me than the last one so.
Things done
Added a beta build (modeled after the way atom/atom-beta handles it) and fixed deprecation warnings related to phpfpm socket and extraConfig that showed up in nixos-unstable a while ago.
@mmilata added a test for matomo and @aanderse helped me figure out how to make it work on matomo-beta as well. Thanks all!
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @florianjacob