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

systemd: remove local-fs.target, which was accidentially added to systemd 243 #69253

Closed
wants to merge 2 commits into from

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Sep 22, 2019

Motivation for this change
commit 528ac1429a8b086788926931c0a7e3386fd93183
Author: Florian Klink <flokli@flokli.de>
Date:   Sun Sep 22 15:06:03 2019 +0200

    systemd: don't use a custom repo anymore, but apply patches on top of systemd's sources
    
    Keeping a custom branch and cherry-picking patches on top of vanilla
    systemd has proven to be error-prone and hard to sync far too often.
    We ship a chunk of patches in nixpkgs, and slowly move them to upstream
    or fix inside NixOS where possible.
    
    This patch simply inlines all the current commits on top of vanilla
    systemd 243.

commit 85dd4eadda5ea5e7aef5998bdeac95ee14ddc5c6 (flokli/systemd-local-fs, systemd-local-fs)
Author: Florian Klink <flokli@flokli.de>
Date:   Sun Sep 22 15:08:46 2019 +0200

    systemd: drop 0002-sysinit.target-Drop-the-dependency-on-local-fs.targe.patch
    
    We removed that patch in our 242 branch, but somehow it landed in 243
    back again.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 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.
Notify maintainers

cc @

… systemd's sources

Keeping a custom branch and cherry-picking patches on top of vanilla
systemd has proven to be error-prone and hard to sync far too often.
We ship a chunk of patches in nixpkgs, and slowly move them to upstream
or fix inside NixOS where possible.

This patch simply inlines all the current commits on top of vanilla
systemd 243.
…ge.patch

We removed that patch in our 242 branch, but somehow it landed in 243
back again.
@flokli
Copy link
Contributor Author

flokli commented Sep 22, 2019

This needs to be cherry-picked to 19.09 too, as we now assume local-fs.target is back into sysinit.target.

@andir
Copy link
Member

andir commented Sep 22, 2019 via email

@Mic92
Copy link
Member

Mic92 commented Sep 22, 2019

I am OK with dropping NixOS/systemd, when we get to a significant lower patch count. But not in the current state.

@arianvp
Copy link
Member

arianvp commented Sep 23, 2019

Heya all. Just to give some context to this patch. It was a result of the Systemd Hackathon that I, @flokli, fpletz, globin and more attended. First of all, note that it consists of two things. Making sure local-fs.target is part of DefaultDependencies= again, and moving to a patchset instead of a fork. I think we can all agree that number one is desirable, as currently services on master might be started up before the local file system is ready; which is not ideal.

About the second commit:

We had a long discussion with the core maintainers about some bugs we're facing with systemd (e.g. the machinectl stuff, and predictible interfaces names) but we also went through all our commits that we have one by one and discussed them pretty thoroughly. They convinced us that many of these patches can either be upstreamed or dropped because there is behaviour in systemd that we didn't know of that can actually implement the semantics we want.

To give some examples:

  • We have a few patches that patch up upstream systemd units, but those can (we think) all be replaced with drop-in units in the NixOS module system. At least, we couldn't find a good reason why that wouldn't be possible.

  • We have a patch that makes sure not to get /nix/store remounted during boot. However, lennart showed us that we can add DefaultDependencies=no and some other options to its nix-store.mount unit to achieve the same behaviour.

  • The patch that adds factorypkgconfdir to the meson.build they were not opposed to upstreaming.

  • The patch that adds ROOTPREFIX the lookup path can with quite some likelihood be upstreamed.

  • Patches that don't enumerate certain directories such as /usr/lib they advised us on dropping wherever possible because we missed some places in the codebase anyway where we're still doing the enumeration and in almost all places where this happens the absence of the enumerated directories is not fatal.

All in all this can drastically decrease the amount of patches that we maintain on top of systemd - probably to the point it is only one or two patches - and will make it a lot easier to follow systemd-stable as our base repository. I think this will make life for the maintainer easier.

Flokli introduced this change because he had to touch the systemd codebase, and this seemed like an easy way to introduce both his desired local-fs change, and experiment with having a patchset on top of systemd, and hackathon was limited time so this was the fastest for us to experiment with both goals

Whether we want to do this now, or whether we want to discuss the patchset part more thoroughly and remove that for now, is both fine to me.

@andir if you're more comfortable with not having the "move to patchset" part in this PR, then I think we should remove it for now and rebase our changes to nixos/systemd, as I think getting the local-fs change, which currently is important to get 19.09 in a stable shape, is more important than bikeshedding about release processes. However we should make this decision quickly, as 19.09 is right around the corner and we do need the local-fs change in.

P.S.
You remark about "wanting to get removed from the maintainer list" made me feel morally obliged to also add myself to this list so that the systemd package has a bit more bandwidth. Regardless of whether you decide to leave it or not =) I think others are willing to add themselves to it too. I didn't realise the list was so small.

@Mic92
Copy link
Member

Mic92 commented Sep 23, 2019

I would put these two changes in separate pull requests. I have pushed a revert to the current systemd 243 branch: NixOS/systemd@ccec67c that we can backport to 19.09 as well.
In a second pull request you take the time to actually reduce the number of patches to a reasonable number. This is long-term, organizational change and there is no need to rush.

@Mic92
Copy link
Member

Mic92 commented Sep 23, 2019

Also see #69284

@arianvp
Copy link
Member

arianvp commented Sep 23, 2019

Perfect. Feel free to close this PR then.

@edolstra
Copy link
Member

BTW I disagree with this:

systemd: don't use a custom repo anymore, but apply patches on top of systemd's sources

Keeping a custom branch and cherry-picking patches on top of vanilla systemd has proven to be error-prone and hard to sync far too often.

Git is a much better patch manager than manually applying patches in Nixpkgs. Doing a git rebase is much easier than manually unpacking the latest upstream sources, applying the patches, fixing the conflicts and regenerating the patches.

@edolstra
Copy link
Member

Indeed this PR would be a step backwards: it restores the bad situation that we used to have, with lots of patches in the Nixpkgs tree rather than under proper version management control.

@arianvp
Copy link
Member

arianvp commented Sep 23, 2019

Fair enough. However I think it's still a noble goal to be as "upstream as possible" and talking to the systemd developers gave me a lot of confidence that we can get rid of many of the patches that we currently have. But that doesn't mean those patches have to live in nixpkgs as a list of patches of course. Those are two orthogonal things. Our fork will also be easier to manage even if we keep it in version control but the amount of patches is smaller.

However, in the event that we're able to reduce that patch count to a single patch or two, I think having the patch in nixpkgs instead is desirable. However that is all future work. Step one would be reducing the patch count, which will improve the situation either way.

@flokli
Copy link
Contributor Author

flokli commented Sep 23, 2019

With #69284 and #69285 merged in, let's close this PR.

I'll reopen a rebased PR similar to this one, gradually reducing the amount of patches - We can then decide on whether we'll merge that in as-is to keep the history, or squash together.

@flokli flokli closed this Sep 23, 2019
@flokli flokli deleted the systemd-local-fs branch September 23, 2019 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants