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

epgstation: init at 1.7.4 #49413

Merged
merged 3 commits into from
Sep 14, 2020
Merged

epgstation: init at 1.7.4 #49413

merged 3 commits into from
Sep 14, 2020

Conversation

midchildan
Copy link
Member

@midchildan midchildan commented Oct 29, 2018 •
edited
Loading

Motivation for this change

EPGStation is a DVR software for ISDB television. This depends on #48740.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

@midchildan midchildan changed the title Add epgstation epgstation: init at 1.1.8 Oct 29, 2018
@GrahamcOfBorg GrahamcOfBorg 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 Oct 29, 2018
@GrahamcOfBorg GrahamcOfBorg added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 4, 2018
@GrahamcOfBorg GrahamcOfBorg removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 4, 2018
@midchildan midchildan force-pushed the add-epgstation branch 2 times, most recently from f37d59c to a87df13 Compare November 11, 2018 05:55
@midchildan midchildan force-pushed the add-epgstation branch 2 times, most recently from 4be1157 to bd49f5b Compare November 11, 2018 11:32
@midchildan midchildan changed the title epgstation: init at 1.1.8 epgstation: init at 1.2.7 Dec 29, 2018
@marvin-mk2 marvin-mk2 bot added the awaiting_reviewer (old Marvin label, do not use) label Sep 1, 2020
glittershark
glittershark approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@glittershark glittershark left a comment

Choose a reason for hiding this comment

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

code LGTM, though I would like a maintainer to look at it as well.

Also, looks like you need to rebase -i --autosquash

@midchildan
Copy link
Member Author

Thanks. I've squashed the commits.

@marvin-mk2 marvin-mk2 bot added needs_reviewer (old Marvin label, do not use) and removed awaiting_reviewer (old Marvin label, do not use) labels Sep 4, 2020
@marvin-mk2 marvin-mk2 bot requested a review from ryantm September 5, 2020 04:18
@marvin-mk2 marvin-mk2 bot added awaiting_reviewer (old Marvin label, do not use) needs_reviewer (old Marvin label, do not use) and removed needs_reviewer (old Marvin label, do not use) awaiting_reviewer (old Marvin label, do not use) labels Sep 5, 2020
@marvin-mk2 marvin-mk2 bot requested a review from timokau September 10, 2020 16:47
@marvin-mk2 marvin-mk2 bot added awaiting_reviewer (old Marvin label, do not use) and removed needs_reviewer (old Marvin label, do not use) labels Sep 10, 2020
timokau
timokau approved these changes Sep 11, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

The remaining nits are so minor that I don't want to hold this up even longer over them. Great work!

nixos/modules/services/video/epgstation/default.nix
};
};

defaultPassword = "INSECURE_GO_CHECK_CONFIGURATION_NIX\n";
Copy link
Member

Choose a reason for hiding this comment

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

Why the newline?

Copy link
Member Author

Choose a reason for hiding this comment

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

pkgs.writeText didn't actually write anything without the newline.

@timokau timokau mentioned this pull request Sep 11, 2020
Merged
11 tasks
@timokau
Copy link
Member

timokau commented Sep 11, 2020

Oh, actually needs a rebase anyway. Would it be possible to do the node update and the epgstation update in separate commits? Not really important though if its a lot of effort.

@midchildan
Copy link
Member Author

Unfortunately it can't. All node packages would have to be upgraded simultaneously. I'll just do a rebase now.

@midchildan
Copy link
Member Author

rebased :-)

@timokau
Copy link
Member

timokau commented Sep 14, 2020

Unfortunately it can't. All node packages would have to be upgraded simultaneously. I'll just do a rebase now.

At least for vim plugins, the way we usually handle new inclusions is first update all existing packages, then add the new package and run another "update" (which will then only add the new package). Anyway, not very important. Thank you for your patience and the great work here!

@timokau timokau merged commit 8e13dae into NixOS:master Sep 14, 2020
@midchildan midchildan deleted the add-epgstation branch September 14, 2020 13:42
@midchildan midchildan mentioned this pull request Sep 18, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennofs bennofs bennofs left review comments

@infinisil infinisil infinisil requested changes

@glittershark glittershark glittershark approved these changes

@timokau timokau timokau approved these changes

@ryantm ryantm Awaiting requested review from ryantm

Assignees
No one assigned
Labels
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/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes awaiting_reviewer (old Marvin label, do not use) marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@midchildan @nixos-discourse @timokau @glittershark @bennofs @infinisil @ryantm @GrahamcOfBorg