Skip to content

flood: init at 3.1.0 #102552

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
Closed
wants to merge 1 commit into from
Closed

flood: init at 3.1.0 #102552

wants to merge 1 commit into from
+9,698 −0

Conversation

thiagokokada
Copy link
Contributor

@thiagokokada thiagokokada commented Nov 2, 2020 •
edited
Loading

Motivation for this change

This PR adds https://github.com/jesec/flood. As described in the GitHub page:

A web UI for rTorrent with a Node.js backend and React frontend.

It goes well with the recently(ish) addition of rtorrent as a daemon: #83287

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.

Sorry, something went wrong.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 2, 2020
@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 102552 run on x86_64-linux 1

1 package built:
  • flood

@thiagokokada
Copy link
Contributor Author

Fixed the license and rebased at 4440ec3.

@thiagokokada
Copy link
Contributor Author

/marvin opt-in
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2 label Nov 7, 2020
@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 7, 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.

@marvin-mk2 marvin-mk2 bot added the needs_reviewer (old Marvin label, do not use) label Nov 7, 2020
@thiagokokada
Copy link
Contributor Author

/status awaiting_reviewer

@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 Nov 7, 2020
@thiagokokada
Copy link
Contributor Author

@SuperSandro2000 can you review again?

@thiagokokada
Copy link
Contributor Author

/status needs_reviewer

@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 Nov 7, 2020
@SuperSandro2000
Copy link
Member

@SuperSandro2000 can you review again?

I am not that familiar with node packages.

@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 Nov 8, 2020
@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 11, 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.

3 similar comments
@marvin-mk2
Copy link

marvin-mk2 bot commented Nov 14, 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 bot commented Nov 17, 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 bot commented Nov 20, 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
timokau requested changes Nov 21, 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.

Sorry for the marvin bug. I'm also not very familiar with node packages. @cideM could you have a look?

pkgs/applications/networking/flood/default.nix
{ pkgs, nodejs, nodePackages, stdenv, lib, ... }:

let
nodePackagesLocal = import ./node-composition.nix {
Copy link
Member

Choose a reason for hiding this comment

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

Can this reuse the shared node packages in nixpkgs? 9000 lines for one package is a bit much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timokau I based this on other node packages that are applications too, like this one: https://github.com/NixOS/nixpkgs/blob/e7fe577d9803885d1191c6612b95c246cb605dde/pkgs/development/web/newman/node-packages.nix. There is other examples in nixpkgs too for node packages that brings its full dependency tree.

Why I preferred this approach in this package is that like many things in node ecosystem this package dependencies are always changing. Current master already have a completely different set of dependencies.

Copy link
Contributor

@cideM cideM Nov 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

like many things in node ecosystem this package dependencies are always changing

But if you were to just add flood to the shared node packages, generate.sh should just add whatever dependencies are required by flood specifically, while using shared dependencies if available, no?

I'm not actually very familiar with the Node part of Nixpkgs either, I just did this one PR where I merged a lot of Node PRs with merge conflicts. But here I see no reason why flood can't be handled like all other Node tools.

In fact this PR https://github.com/NixOS/nixpkgs/pull/86776/files seems to try to remove all those other node-packages.json files

Copy link
Member

Choose a reason for hiding this comment

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

For context: All the node package definitions actually increase the repository size over time since they get regenerated a lot and each version is kept in git history.

@marvin-mk2 marvin-mk2 bot added awaiting_changes (old Marvin label, do not use) and removed awaiting_reviewer (old Marvin label, do not use) awaiting_changes (old Marvin label, do not use) labels Nov 21, 2020
@marvin-mk2 marvin-mk2 bot added the awaiting_reviewer (old Marvin label, do not use) label Nov 22, 2020
@thiagokokada
Copy link
Contributor Author

thiagokokada commented Nov 22, 2020 via email

@thiagokokada thiagokokada mentioned this pull request Nov 24, 2020
Merged
10 tasks
@thiagokokada
Copy link
Contributor Author

Closed by: #104735

@thiagokokada thiagokokada deleted the flood-init branch November 24, 2020 15:57
@thiagokokada thiagokokada mentioned this pull request Dec 31, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timokau timokau

@cideM cideM

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 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.

None yet
4 participants
@thiagokokada @SuperSandro2000 @timokau @cideM