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

Jump to bottom

systemd: v243.3 -> v243.4 #74207

Closed
wants to merge 2 commits into from
Closed

systemd: v243.3 -> v243.4 #74207

wants to merge 2 commits into from

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Nov 25, 2019

This bumps systemd to the v243.4 stable release.

#74026 already bumped to 243.3, but that was not sufficient enough to include the fixes mentioned in #73504 (comment).

I'd appreciate some thorough looks, especially given upstream also did some changes in src/timedate/timedated.c, which had to be treated manually while merging in the v243.4 branch.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@flokli flokli requested review from Mic92 and andir November 25, 2019 22:15
flokli
flokli commented Nov 25, 2019
View reviewed changes
pkgs/os-specific/linux/systemd/default.nix
@@ -36,10 +36,10 @@ in stdenv.mkDerivation {
# When updating, use https://github.com/systemd/systemd-stable tree, not the development one!
# Also fresh patches should be cherry-picked from that tree to our current one.
src = fetchFromGitHub {
owner = "NixOS";
owner = "flokli";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll push this to NixOS/systemd (and change back the expression) once everybody agrees with the changes there.

Copy link
Member

Choose a reason for hiding this comment

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

The commit should be reachable through the NixOS repo as well as long as it exists somewhere in the fork network, so this shouldn't even be necessary. (Here this feature is convenient, but it can also be bad, see e.g. 7b4c9e9d012)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I know. Still, I want to push it properly.

@ofborg ofborg bot requested a review from edolstra November 25, 2019 22:57
@andir
Copy link
Member

andir commented Nov 25, 2019 •
edited

IF you are referring to flokli/systemd@cc103c7 then that shouldn't matter for us since we disabled manual changing of timezones anyway.

In general please provide links to those places that did have to be manually changed instead of sending people on a hunt when asking for feedback. GitHub is terrible at displaying merges and which commits were introduced in which merge....

@flokli
Copy link
Contributor Author

flokli commented Nov 25, 2019

Yes, that was what I meant, and I agree it shouldn't matter for NixOS.

@flokli flokli mentioned this pull request Nov 25, 2019
Closed
@lheckemann
Copy link
Member

IF you are referring to flokli/systemd@cc103c7 then that shouldn't matter for us since we disabled manual changing of timezones anyway.

Only if time.timeZone is set! Otherwise timezones can still be set using timedatectl set-timezone— I use this since I don't consider my laptop's current physical location as part of its config.

@flokli
Copy link
Contributor Author

flokli commented Nov 27, 2019

Only if time.timeZone is set! Otherwise timezones can still be set using timedatectl set-timezone— I use this since I don't consider my laptop's current physical location as part of its config.

I'm not sure if this is properly supported at all.

Does this mean this PR should be done differently?

@lheckemann
Copy link
Member

lheckemann commented Nov 28, 2019 •
edited

I'm not sure if this is properly supported at all.

Well, it's worked since #26608 ;)

Does this mean this PR should be done differently?

I'm not sure, haven't looked at the actual code yet. I just wanted to correct the assumption that the timezone is always fixed on nixos for now :)

EDIT: Just saw that I introduced a test with the change, so if nixos/tests/timezone.nix still passes it should all be good.

@lheckemann
Copy link
Member

This breaks nixos/tests/timezone.nix attr timezone-imperative. I can have a look at it tomorrow.

@FRidh FRidh added this to Needs review in Staging Nov 30, 2019
@flokli
Copy link
Contributor Author

flokli commented Dec 2, 2019

@lheckemann ping ;-)

@tomfitzhenry tomfitzhenry mentioned this pull request Dec 4, 2019
Merged
10 tasks
@flokli
Copy link
Contributor Author

flokli commented Dec 6, 2019 •
edited

@lheckemann ping ;-)

This is required to fix #73504 and blocking #73505.

@FRidh FRidh added this to the 20.03 milestone Dec 10, 2019
@lheckemann
Copy link
Member

mayflower/systemd@17b4843 fixes it, should probably be squashed into flokli/systemd@cc103c7 with a corresponding explanation

@flokli
Copy link
Contributor Author

flokli commented Dec 14, 2019

@lheckemann feel free to prepare another branch at github.com/mayflower/systemd and take over this PR - I think you know best how to wire this into the existing commit history ;-)

@lheckemann lheckemann mentioned this pull request Dec 21, 2019
Merged
3 tasks
@flokli
Copy link
Contributor Author

flokli commented Dec 27, 2019

closing in favour of #76134.

@flokli flokli closed this Dec 27, 2019
Staging automation moved this from Needs review to Done Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lheckemann lheckemann lheckemann left review comments

@globin globin globin approved these changes

@Mic92 Mic92 Awaiting requested review from Mic92

@andir andir Awaiting requested review from andir

@edolstra edolstra Awaiting requested review from edolstra

Assignees

@lheckemann lheckemann

Labels
10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 11.by: package-maintainer
Projects
Staging
  
Done
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@flokli @andir @lheckemann @globin @FRidh