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

Twitch-Chat-Downloader: init with dependencies #71166

Closed
wants to merge 5 commits into from
Closed

Twitch-Chat-Downloader: init with dependencies #71166

wants to merge 5 commits into from

Conversation

strager
Copy link
Contributor

@strager strager commented Oct 15, 2019 •
edited
Loading

Motivation for this change
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 nix-review --run "nix-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.
Notify maintainers

cc @berdario @thanegill

@ofborg ofborg bot requested a review from thanegill October 15, 2019 05:17
@strager strager marked this pull request as ready for review October 15, 2019 18:57
@strager strager requested a review from FRidh as a code owner October 15, 2019 18:57
@strager strager changed the title Twitch chat downloader Twitch-Chat-Downloader: init with dependencies Oct 15, 2019
jonringer
jonringer reviewed Nov 20, 2019
View reviewed changes
pkgs/development/python-modules/rx/3.0.nix Outdated

buildPythonPackage rec {
pname = "Rx";
version = "3.0.1";
Copy link
Contributor

Choose a reason for hiding this comment

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

We can only have 1 version of package in nixpkgs, this is to avoid situations where two packages bring into the two versions as dependencies, only one of them will be able to be used at runtime.

If you're freezing 1.6 for python2, then that's different, and you will need to add something similar to:

  imbalanced-learn =
    if isPy27 then
      callPackage ../development/python-modules/imbalanced-learn/0.4.nix { }
    else
      callPackage ../development/python-modules/imbalanced-learn { };

this way, only version is visible for a given interpreter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like nothing uses Rx version 1.6 currently. I'll upgrade it in-place instead of adding a new version.

pkgs/tools/networking/twitch-chat-downloader/default.nix Outdated

# Twitch-Chat-Download has no tests at all. Sanity check the command manually.
checkPhase = ''
"$out"/bin/tcd --help >/dev/null || exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

The || exit 1 will only be called if the previous command failed, just return the original value

Suggested change
"$out"/bin/tcd --help >/dev/null || exit 1
"$out"/bin/tcd --help >/dev/null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. For some reason, I assumed -e was not in effect.

FRidh
FRidh requested changes Nov 20, 2019
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

I can think of absolutely no good reason why a package would want to import from pipenv.

I had a quick look at the setup.py and what is done there is just ridiculous. They use pipenv to create an env, an then use pipenv to convert the locked dependencies for setuptools. I get why they do that as it simplifies some of their work but its not what should be done in a setup.py. That whole part needs to be patched out before it can go in Nixpkgs.

Pipenv should not be module but remain an application.

strager
strager commented Nov 21, 2019
View reviewed changes
Copy link
Contributor Author

@strager strager left a comment

Choose a reason for hiding this comment

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

I can think of absolutely no good reason why a package would want to import from pipenv.

I had a quick look at the setup.py and what is done there is just ridiculous. They use pipenv to create an env, an then use pipenv to convert the locked dependencies for setuptools. I get why they do that as it simplifies some of their work but its not what should be done in a setup.py. That whole part needs to be patched out before it can go in Nixpkgs.

Pipenv should not be module but remain an application.

Good points. Patching the pipenv-using packages was easy, so I'll do that instead of exposing pipenv as a library.

@strager strager requested a review from FRidh December 8, 2019 01:40
@strager
Copy link
Contributor Author

strager commented Feb 18, 2020

not sure if it shows on your UI, but on main there's a arrow to the right which I can expand and dismiss.

I think I see the "v" arrow you're talking about. When I click it, a panel opens with the text "FRidh requested changes" and a "..." button. Clicking the "..." button shows a menu with one option: "See review". From there, I don't see any button labelled "dismiss".

@jonringer
Copy link
Contributor

not sure if it shows on your UI, but on main there's a arrow to the right which I can expand and dismiss.

I think I see the "v" arrow you're talking about. When I click it, a panel opens with the text "FRidh requested changes" and a "..." button. Clicking the "..." button shows a menu with one option: "See review". From there, I don't see any button labelled "dismiss".

Selection_009

this is what mine looks like

jonringer
jonringer reviewed Feb 20, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

otherwise LGTM, assuming everything has some tests

pkgs/tools/networking/twitch-chat-downloader/default.nix Outdated Show resolved Hide resolved
@strager
Copy link
Contributor Author

strager commented Feb 20, 2020

otherwise LGTM, assuming everything has some tests

I don't think any of these packages have significant tests of their own.

@strager
Copy link
Contributor Author

strager commented Feb 20, 2020

not sure if it shows on your UI, but on main there's a arrow to the right which I can expand and dismiss.

I think I see the "v" arrow you're talking about. When I click it, a panel opens with the text "FRidh requested changes" and a "..." button. Clicking the "..." button shows a menu with one option: "See review". From there, I don't see any button labelled "dismiss".

Selection_009

this is what mine looks like

I see only "See review":

Screen Shot 2020-02-19 at 23 33 07

@jonringer
Copy link
Contributor

PetterKraabol/Twitch-Python#13 was closed, if you want to wait until the next release, and update this, then I think we can have another go

@PetterKraabol
Copy link

Hi, I've removed the pipenv dependecy from setup.py in twitch-python and twitch-chat-downloader.

@strager
Copy link
Contributor Author

strager commented May 15, 2020

Gross patch removed. (Thanks @jonringer and @PetterKraabol!) This PR is ready for review again.

@strager
Copy link
Contributor Author

strager commented May 16, 2020

graphql-core's build seems to be broken. I'll investigate.

@strager strager mentioned this pull request May 16, 2020
Merged
10 tasks
@FRidh FRidh requested a review from jonringer May 17, 2020 10:53
@FRidh
Copy link
Member

FRidh commented May 17, 2020

@GrahamcOfBorg build twitch-chat-downloader

@strager
graphql-core: 2.3.1 -> 3.1.0
115fbc3 
I want to upgrade rx (a Python library) from version 1.6 to version 3.0.
graphql-core version 2.3.1 requires rx version <2. This prevents me from
upgrading rx.

Upgrade graphql-core to the latest release, removing its dependency on
rx entirely.

Unfortunately, graphql-server-core (which depends on graphql-core) does
not have a release which works with this latest version of graphql-core.
Because graphql-server-core isn't used anywhere yet [1], disable the
package to unblock me upgrading rx.

[1] NixOS#80828 (comment)
@strager
pythonPackages.rx: 1.6 -> 3.0.1
da47227 
Rx version 1.6 is incompatible with Rx version 3.x. I want to add a
package which depends on Rx version 3.x (twitch-python). Upgrade Rx
to version 3.0.1.

From what I can tell, no packages use Rx version 1.6, so this commit
shouldn't break any existing builds (aside from possibly Rx itself).
@strager
maintainers: add strager
07b72e9
@strager
pythonPackages.twitch-python: init at 0.0.17
a1d2156 
I want to add Twitch-Chat-Downloader to Nixpkgs. Twitch-Chat-Downloader
depends on the twitch-python package. Add its dependency.
@strager
Twitch-Chat-Downloader: init at 3.2.0
97e1109 
Add Petter Kraabøl's Twitch-Chat-Downloader tool to Nixpkgs, enabling
people to archive Twitch [1] messaging history.

[1] https://www.twitch.tv/
@ofborg ofborg bot requested a review from kamadorueda May 20, 2020 02:49
@marsam
Copy link
Contributor

marsam commented Feb 12, 2021

superseded by #112712

@marsam marsam closed this Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thanegill thanegill Awaiting requested review from thanegill

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@kamadorueda kamadorueda Awaiting requested review from kamadorueda

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: python 8.has: clean-up 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@strager @jonringer @PetterKraabol @FRidh @marsam @ryantm