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

nixos/network-interfaces: remove network-interfaces.target #272169

Merged
merged 2 commits into from
Jan 14, 2024
Merged

nixos/network-interfaces: remove network-interfaces.target #272169

merged 2 commits into from
Jan 14, 2024

Conversation

nyabinary
Copy link
Contributor

@nyabinary nyabinary commented Dec 5, 2023 •
edited
Loading

Description of changes

Seemingly nothing is using this and network-interfaces was deprecated all the way back in 2016: #18491

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 5, 2023
@nyabinary nyabinary changed the title Drop deprecated and unused network-interfaces target drop deprecated and unused network-interfaces target Dec 5, 2023
@pbsds
Copy link
Member

pbsds commented Dec 5, 2023

This will be a silent failure for many people.

Can/will this be caught at eval time?

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Dec 5, 2023 •
edited
Loading

Then definitely add a release note entry.

I don't think it is as big. Some of the results are stone age old nixpkgs copies.

When we add -is:fork then we are already down to just below 200 https://github.com/search?type=code&q=-repo%3Anixos%2Fnixpkgs+lang%3Anix+network-interfaces.target+-is%3Afork

Edit: even with that there seem to be many repos which truly are just old nixpkgs.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 5, 2023
@nyabinary nyabinary force-pushed the remove-deprecated-network-interfaces branch from f147ec6 to 0768c07 Compare December 5, 2023 03:35
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Dec 5, 2023
@nyabinary nyabinary changed the title drop deprecated and unused network-interfaces target nixos/network-interfaces: remove network-interfaces.target Dec 5, 2023
@nyabinary nyabinary force-pushed the remove-deprecated-network-interfaces branch from 0768c07 to 8f19e8e Compare December 5, 2023 03:48
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 5, 2023
@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 5, 2023
@nyabinary nyabinary force-pushed the remove-deprecated-network-interfaces branch from 8f19e8e to d7c094a Compare December 5, 2023 12:35
@delroth delroth removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 5, 2023
@delroth delroth added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Dec 5, 2023
@nyabinary nyabinary force-pushed the remove-deprecated-network-interfaces branch from d7c094a to aa97325 Compare December 5, 2023 20:28
@delroth delroth removed the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Dec 5, 2023
lilyinstarlight
lilyinstarlight approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

This is a good idea, thank you!

I looked through some of the downstream repos and I see a few that are still active (including nixops depending on this unit...), so maybe we should make a short post at https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574 to notify people?

@delroth delroth added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Dec 5, 2023
@delroth delroth added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Dec 20, 2023
philiptaron
philiptaron approved these changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

What's needed in order to merge this?

@nyabinary nyabinary requested a review from a team as a code owner December 24, 2023 12:58
@mkg20001 mkg20001 force-pushed the remove-deprecated-network-interfaces branch from 74ff1c1 to 2c0219d Compare December 24, 2023 13:02
@mkg20001
Copy link
Member

You would have to add an assertion in that particular place, then it should be good.

Added one

@mkg20001 mkg20001 force-pushed the remove-deprecated-network-interfaces branch 2 times, most recently from bad24d2 to 9176fbf Compare December 24, 2023 13:05
@nyabinary
Copy link
Contributor Author

You would have to add an assertion in that particular place, then it should be good.

Added one

Thank you :)

@mkg20001 mkg20001 force-pushed the remove-deprecated-network-interfaces branch from 08486e1 to dbbab5d Compare December 24, 2023 15:28
@mkg20001 mkg20001 mentioned this pull request Dec 24, 2023
Closed
13 tasks
@mkg20001
Copy link
Member

Created pr #276493 for warning on 23.11 release

@mkg20001
Copy link
Member

I'd need a review on the assertions. If it's good or a bit too hacky what I wrote. Then I'll merge.

@nyabinary
Copy link
Contributor Author

@mkg20001

It should just be use network.target instead for the release notes. Omitting the “the” from the sentence, right?

@mkg20001 mkg20001 force-pushed the remove-deprecated-network-interfaces branch from dbbab5d to b5b0a42 Compare December 25, 2023 17:05
ElvishJerricco
ElvishJerricco reviewed Jan 1, 2024
View reviewed changes
nixos/modules/system/boot/systemd.nix
map (message: {
assertion = false;
inherit message;
}) (concatLists [
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertion is very confusingly written. Why is there a concatLists for a list with a single element? Why is it mapping a list made with optional to an assertion with assertion = false instead of just putting the condition in the assertion field?

Copy link
Member

Choose a reason for hiding this comment

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

assertion = false

I don't know the impact of adding a lot of assertions (as there would be one for every system service)

concatLists

This is copied from the warnings above, to support multiple assertions later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any further objections? @ElvishJerricco

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this was extremely weirdly written and should have been cleaned up, but I wasn't going to block the PR on it

@nyabinary nyabinary added the needs_merger (old Marvin label, do not use) label Jan 3, 2024
@nyabinary nyabinary force-pushed the remove-deprecated-network-interfaces branch from b5b0a42 to 1062655 Compare January 12, 2024 16:16
@nyabinary
Copy link
Contributor Author

Fixed merge conflict should be ready to merge I think(?)

@mkg20001 mkg20001 force-pushed the remove-deprecated-network-interfaces branch from 1062655 to c0ef1f9 Compare January 12, 2024 16:29
@mkg20001
Copy link
Member

@nyabinary You force-pushed away the assertion, re-added that.

@nyabinary
Copy link
Contributor Author

nyabinary commented Jan 12, 2024 •
edited
Loading

@nyabinary You force-pushed away the assertion, re-added that.

Oops sorry and thanks :3

@symphorien symphorien merged commit e52366c into NixOS:master Jan 14, 2024
19 checks passed
@nyabinary nyabinary deleted the remove-deprecated-network-interfaces branch January 14, 2024 15:11
@RaitoBezarius
Copy link
Member

If we can believe this stuff: https://github.com/NixOS/nixpkgs/runs/20435679228 — this had visibly no impact on evaluation performance, phew, module system is great!

@SuperSandro2000
Copy link
Member

That's probably because the expensive part, evaluating things, is already done in warnings and you only see a difference if both would be dropped. According to testing a year ago evaluating warnings takes 4s on a noop rebuild.

@nyabinary nyabinary removed the needs_merger (old Marvin label, do not use) label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkg20001 mkg20001 mkg20001 approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@ElvishJerricco ElvishJerricco ElvishJerricco left review comments

@mweinelt mweinelt mweinelt left review comments

@NickCao NickCao NickCao approved these changes

@Mindavi Mindavi Mindavi approved these changes

@lilyinstarlight lilyinstarlight lilyinstarlight approved these changes

@pbsds pbsds pbsds approved these changes

@philiptaron philiptaron philiptaron approved these changes

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: clean-up 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.