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

mosquitto: 1.6.2 -> 1.6.3 #36865

Closed
wants to merge 1 commit into from
Closed

mosquitto: 1.6.2 -> 1.6.3 #36865

wants to merge 1 commit into from

Conversation

peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Mar 12, 2018 •
edited

Motivation for this change

mosquitto provides very helpful logging which was nowhere to be found when not run in the foreground so let's run it in the foreground.

Also ~~~update it from 1.4.14 to 1.4.15 and~~~ build it with cmake on all platforms and quote the password so we can accept one with spaces.

I really want to add some tests for this but I haven't done that yet.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

Mic92
Mic92 reviewed Mar 12, 2018
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

both changes should be backported to 18.03 as well.

Mic92
Mic92 reviewed Mar 12, 2018
View reviewed changes
nixos/modules/services/networking/mosquitto.nix Outdated
ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
PIDFile = "/run/mosquitto/pid";
PrivateTmp = true;
Copy link
Member

Choose a reason for hiding this comment

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

What does it put into /tmp?

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 don't know if it does put anything there but it's one of those "just to be on the safe side" things. In any case, with DynamicUser = true; this is set anyway.

Mic92
Mic92 reviewed Mar 12, 2018
View reviewed changes
pkgs/servers/mqtt/mosquitto/default.nix Outdated
# http://www.eclipse.org/legal/epl-v10.html (free software, copyleft)
license = stdenv.lib.licenses.epl10;
license = licenses.epl10;
maintainer = with maintainers; [ peterhoeg ];
Copy link
Member

Choose a reason for hiding this comment

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

maintainers = instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@fpletz fpletz added this to the 18.03 milestone Mar 12, 2018
@peterhoeg peterhoeg changed the title nixos mosquitto: run in foreground so we get logging data nixos mosquitto: run in foreground so we get logging data [WIP] Mar 13, 2018
Mic92
Mic92 reviewed Mar 13, 2018
View reviewed changes
nixos/modules/services/networking/mosquitto.nix Outdated
Type = "forking";
User = "mosquitto";
Group = "mosquitto";
DynamicUser = true;
Copy link
Member

Choose a reason for hiding this comment

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

Right with the new systemd it automatically chown everything on start.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed.

Mic92
Mic92 reviewed Mar 15, 2018
View reviewed changes
pkgs/servers/mqtt/mosquitto/default.nix Outdated

for d in lib lib/cpp src ; do
substituteInPlace $d/CMakeLists.txt \
--replace /sbin/ldconfig ldconfig
Copy link
Member

Choose a reason for hiding this comment

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

Oh man, why does debian still no put /sbin in the user's PATH. This is just broken.

@peterhoeg
Copy link
Member Author

Do we need updates to the release notes due to a config option being removed as well as no longer using a system defined user?

@Mic92
Copy link
Member

Mic92 commented Mar 19, 2018

I think removing services.mosquitto.dataDir is the more invasive change and this covered by rename.nix

@Mic92
Copy link
Member

Mic92 commented Mar 19, 2018

I think the bigger problem would be, if someone has changed dataDir from default.

@peterhoeg
Copy link
Member Author

I think the bigger problem would be, if someone has changed dataDir from default.

They also need to have enabled persistence which is off by default for it to be a problem. How has stuff like this been handled in the past?

@Mic92
Copy link
Member

Mic92 commented Mar 19, 2018 •
edited

Release notes and maybe system.stateVersion

@matthewbauer matthewbauer modified the milestones: 18.03, 18.09 Oct 1, 2018
@samueldr samueldr modified the milestones: 18.09, 19.03 Oct 6, 2018
@peterhoeg
Copy link
Member Author

This isn't going to make it into 19.03 but I will get it cleaned up.

@ofborg ofborg bot removed the 6.topic: nixos label Jun 23, 2019
@peterhoeg peterhoeg changed the title nixos mosquitto: run in foreground so we get logging data [WIP] mosquitto: 1.6.2 -> 1.6.3 Jun 24, 2019
@peterhoeg
Copy link
Member Author

Sorry about the branch reuse - that screwed things up.

@peterhoeg peterhoeg closed this Jun 24, 2019
@peterhoeg peterhoeg deleted the u/mosquitto branch June 25, 2019 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

Assignees
No one assigned
Labels
2.status: work-in-progress 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
19.03
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@peterhoeg @Mic92 @matthewbauer @fpletz @samueldr @GrahamcOfBorg