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

Fix NixOS detection #87187

Merged
merged 1 commit into from
Jul 21, 2021
Merged

Fix NixOS detection #87187

merged 1 commit into from
Jul 21, 2021

Conversation

oxalica
Copy link
Contributor

@oxalica oxalica commented Jul 16, 2021 •
edited
Loading

Use /etc/os-release instead of /etc/NIXOS for detection.
The latter one does not exist on NixOS when using tmpfs as root.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2021
@nagisa
Copy link
Member

nagisa commented Jul 16, 2021

r? @nagisa

Under what circumstances might /etc/NIXOS not exist? AFAICT from grepping around nixpkgs this file is ubiquitous.

@oxalica
Copy link
Contributor Author

oxalica commented Jul 16, 2021

Under what circumstances might /etc/NIXOS not exist?

I mentioned it in the code comment. It doesn't exist if you are using tmpfs root (use auto population of /etc on every boot).
/etc/NIXOS is only created in installation script but not in activation script.

Generally I think /etc/os-release should be preferred to do "distro detection", and it's more extensible if we need special treatment for some other distro.

AFAICT from grepping around nixpkgs this file is ubiquitous.

Not many outside installation but do have some. 🤔 Maybe I should also send a PR to nixpkgs to make it always present.

@oxalica oxalica mentioned this pull request Jul 16, 2021
Merged
11 tasks
@nagisa
Copy link
Member

nagisa commented Jul 17, 2021

I mentioned it in the code comment.

Cool, I missed that when quickly looking through the PR.

Would be great if this comment was moved to a commit message and/or the PR description. A code comment here is not particularly useful, I feel. There's no context to a person reading code (especially so the future) as to why the reference to /etc/NIXOS exists at all.

Generally I think /etc/os-release should be preferred to do "distro detection", and it's more extensible if we need special treatment for some other distro.

Sure, though I would argue that extensibility and generality is not really a concern here given that the following fixups are extremely NixOS specific.

r=me once the PR description and/or commit messages are adjusted to include information about tmpfs nixos activations not having this file.

@oxalica
Fix NixOS detection
919a8a5 
Use `/etc/os-release` instead of `/etc/NIXOS`.
The latter one does not exist on NixOS when using tmpfs as root.
@oxalica oxalica force-pushed the fix-nixos-detect branch from a105754 to 919a8a5 Compare July 20, 2021 16:39
@oxalica
Copy link
Contributor Author

oxalica commented Jul 20, 2021

@nagisa Fixed.

@nagisa
Copy link
Member

nagisa commented Jul 20, 2021

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Jul 20, 2021

📌 Commit 919a8a5 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jul 21, 2021
@Dylan-DPC
Rollup merge of rust-lang#87187 - oxalica:fix-nixos-detect, r=nagisa
2cc7e85 
Fix NixOS detection

Use `/etc/os-release` instead of `/etc/NIXOS` for detection.
The latter one does not exist on NixOS when using tmpfs as root.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Jul 21, 2021
Closed
@bors
Copy link
Contributor

bors commented Jul 21, 2021

⌛ Testing commit 919a8a5 with merge db06c87bbb4c488f3fcab9df58f0ccd11cb21f3a...

@bors
Copy link
Contributor

bors commented Jul 21, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 21, 2021
@nagisa
Copy link
Member

nagisa commented Jul 21, 2021

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@GuillaumeGomez GuillaumeGomez mentioned this pull request Jul 21, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 21, 2021
@bors
Auto merge of rust-lang#87347 - GuillaumeGomez:rollup-ke92xxc, r=Guil…
05f2326 
…laumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#87187 (Fix NixOS detection)
 - rust-lang#87206 (avoid temporary vectors/reuse iterators)
 - rust-lang#87230 (Fix docblock <table> overflow)
 - rust-lang#87273 (Recognize bounds on impls as const bounds)
 - rust-lang#87279 (Add comments explaining the unix command-line argument support.)
 - rust-lang#87301 (Fix typo in compile.rs)
 - rust-lang#87311 (Get back the more precise suggestion spans of old regionck)
 - rust-lang#87321 (Add long explanation for E0722)
 - rust-lang#87342 (Add long explanation for E0757)

Failed merges:

 - rust-lang#87270 (Don't display <table> in item summary)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2861265 into rust-lang:master Jul 21, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 21, 2021
@oxalica oxalica deleted the fix-nixos-detect branch July 21, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nagisa nagisa

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
7 participants
@oxalica @rust-highfive @nagisa @bors @rust-log-analyzer @Mark-Simulacrum @rustbot