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

Startx improvements #100774

Merged
merged 5 commits into from Dec 13, 2020
Merged

Startx improvements #100774

merged 5 commits into from Dec 13, 2020

Conversation

@Atemu
Copy link
Member

@Atemu Atemu commented Oct 16, 2020

Motivation for this change

Typo-less and improved version of #80272

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 nixpkgs-review --run "nixpkgs-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.
@Atemu
Copy link
Member Author

@Atemu Atemu commented Oct 16, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Oct 16, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

Copy link
Contributor

@doronbehar doronbehar left a comment

Diff looks good.

nixos/modules/services/x11/xserver.nix Show resolved Hide resolved
@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 17, 2020

The first 2 commits are totally fine I suppose. Regarding the log file's location changing from ~/.xorg.log to /var/log/xorg.log, are we sure that won't cause any issues?

@Atemu
Copy link
Member Author

@Atemu Atemu commented Oct 31, 2020

This changes it to the default upstream Xorg logfile location. The logfile was hard-coded by us to a non-standard location because of an issue someone had in 2012 which was likely caused by Nixpkgs patching the log location to be inside the Nix store, which I also fixed.

The upstream default log location for regular users is inside their home dir, /var/log will only be used when manually running X as root.

I really don't think we need a tmpfiles rule for this. Xorg logs are kilobytes and are rotated automatically AFAICT. They're aren't even going to be there in most cases as you need to manually run X as root.

You probably don't even want them to be deleted automatically as you might want to audit them after a catastrophic failure.

If you sudo startx for some strange reason and care about a few kilobytes in /var/log, you should just delete them manually.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 3, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

Copy link
Contributor

@doronbehar doronbehar left a comment

I approve as all changes make sense. But I didn't test so I don't merge.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 7, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

8 similar comments
@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 10, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 13, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 16, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 19, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 23, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 26, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 29, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Dec 2, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@timokau
Copy link
Member

@timokau timokau commented Dec 2, 2020

Sorry for the spam. Unfortunately I can't look into this in much detail myself right now. @Atemu to what extent have you tested this?

@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 2, 2020

I sucessfully ran i3 with startx and the NixOS test still works too. I also tested running X as root.

Since this requires a rebuild of all Xorg dependent software, I didn't test much further but I could do that now.

Is there a specific scenario you would like to see tested?

Copy link
Member

@timokau timokau left a comment

That's probably sufficient, thank you :) I left some additional feedback.

pkgs/servers/x11/xorg/overrides.nix Outdated Show resolved Hide resolved
nixos/modules/services/x11/xserver.nix Show resolved Hide resolved
nixos/modules/services/x11/xserver.nix Outdated Show resolved Hide resolved
@doronbehar doronbehar requested review from rycee and Ericson2314 Dec 5, 2020
@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Dec 8, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

1 similar comment
@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Dec 12, 2020

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

Copy link
Member

@timokau timokau left a comment

Looks good now, please squash your fixups.

Slightly off-topic: Its usually easier if you explicitly re-request a review (or post a comment) when you have addressed all comments. GitHub doesn't send notifications when threads are resolved.

pkgs/servers/x11/xorg/overrides.nix Show resolved Hide resolved
Atemu added 5 commits Dec 13, 2020
Previously it'd try to use the one under its output path which is read-only of
course
Xorg creates the log-dir in its output path because X crashes if it can't write
to its logfile. On a regular distro, this dir would be installed into the root
to prevent that from happening but with Nix, it sits in the read-only Nix store.
Ironically, when Xorg tries to write here, it fails and crashes.

To make Xorg log to /var/log, we have to stop the build script from trying to
create the log-dir as the sandbox doesn't (and shouldn't) have access to /var.

This creates a runtime dependency on /var when running as root but that should
exist on any Linux system (on NixOS, journald always creates /var/log).

Previously, the startx displayManager required some workarounds for logfiles
which are obsolete now.

patchPhase -> postPatch because overriding the patchPhase prevents patches from
being applied
It makes sense for it to be /dev/null for all the displayManagers but startx,
it needs a different logFile configuration.
This partially reverts bf3d3dd.

I don't know why we weren't getting a default logfile back then but Xorg
definitely provides one now ($XDG_DATA_HOME for regular users and /var/log for
root, see `man Xorg`)
@Atemu Atemu force-pushed the startx-improvements branch from fe02e52 to d3113a6 Dec 13, 2020
@Atemu Atemu changed the base branch from master to staging Dec 13, 2020
@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 13, 2020

Squashed and rebased. Also changed base to staging since this requires a rebuild of all Xorg-server dependent software for something most people don't use.

@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 13, 2020

EditorConfig fail is unrelated, probably an issue in staging.

Copy link
Member

@timokau timokau left a comment

Thank you!

@timokau timokau merged commit d6dba0d into NixOS:staging Dec 13, 2020
16 of 17 checks passed
16 of 17 checks passed
@github-actions[bot]
tests tests
Details
@github-actions[bot]
action
Details
@ofborg[bot]
Evaluation Performance Report Evaluator Performance Report
Details
@github-actions[bot]
Wait for ofborg
Details
@ofborg[bot]
grahamcofborg-eval ^.^!
Details
@ofborg[bot]
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
@ofborg[bot]
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@ofborg[bot]
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d3113a6"; rev="d3113a62b872286369b76c7a73998d1e84f0354d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
@ofborg[bot]
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d3113a6"; rev="d3113a62b872286369b76c7a73998d1e84f0354d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d3113a6"; rev="d3113a62b872286369b76c7a73998d1e84f0354d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d3113a6"; rev="d3113a62b872286369b76c7a73998d1e84f0354d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d3113a6"; rev="d3113a62b872286369b76c7a73998d1e84f0354d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d3113a6"; rev="d3113a62b872286369b76c7a73998d1e84f0354d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d3113a6"; rev="d3113a62b872286369b76c7a73998d1e84f0354d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@ofborg[bot]
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Atemu
Copy link
Member Author

@Atemu Atemu commented Dec 14, 2020

Thank you for the thorough review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants