-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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.
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
popcorntime: 0.3.7.2 -> 0.3.8-3 #8819
Conversation
178baa4
to
4a2315b
Compare
@@ -5588,6 +5588,10 @@ let | |||
|
|||
node_webkit = node_webkit_0_9; | |||
|
|||
nwjs_0_12 = callPackage ../development/tools/node-webkit/nw12.nix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe those package should be on the same file. They are really similar
- node_webkit_0_9
- node_webkit_0_11
- nwjs_0_12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, how do you have this in mind? Make a derivation?
Do note that nwjs has a different install phase than the others.
Edit: or are you saying they should be named the same? All node_webkit_*
instead of nwjs_0_12
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MasseGuillaume Could you explain in a bit more detail what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node_webkit_0_12 = callPackage ../development/tools/node-webkit/default.nix { v = "0.12" }
node_webkit_0_11 = callPackage ../development/tools/node-webkit/default.nix { v = "0.11" }
node_webkit_0_9 = callPackage ../development/tools/node-webkit/default.nix { v = "0.9" }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MasseGuillaume The sources still differ in SHA256. The installPhase differs between 0.12 and 0.11/0.9. That said, nwjs-env
, bits
and part of installPhase
could be generalized. I can do this in a separate PR.
4a2315b
to
afb6c79
Compare
Update: forgot to update the ia32 sha256 of nwjs. This should be fixed now. |
Merge conflict. Please rebase. |
@MasseGuillaume Is |
@bobvanderlinden nope |
Hmm, there is some other issue on master atm.
fails:
PopcornTime indirectly depends on ascli, so I get this same error when building popcorntime. I haven't figured out yet how to solve this. |
afb6c79
to
46d0ce6
Compare
I'd like to merge this, but I have some concerns:
|
https://git.popcorntime.io/popcorntime/desktop.git (with leaveDotGit=1) has a new hash now :-(
Would building without leaveDotGit be a good idea? |
@bjornfor Yes, it is generated with npm2nix. It hasn't occurred to me that it was so big! :| An alternative would be to include a shrinkwrap file (a file that includes all specific versions of the dependencies and hash node_modules in its entirety) and use Other than that, leaveDotGit was there for a reason, but I don't know whether it's still needed. I'll have to check when the build succeeds. At the moment it seems the popcorntime devs have moved their opensubtitles repository, which results in faulty dependency in popcorntime-desktop (cannot find https://git.popcorntime.io/popcorntime/opensubtitles.git). I've opened an issue at popcorntime: https://git.popcorntime.io/popcorntime/desktop/issues/1447 |
is this ready? |
@domenkozar Short answer: no. I've just updated node.nix and the packages to the latest version of Popcorntime (0.3.8-3). However, I still got trouble with npm. I've been tearing my hear out a number of times on this and similar issues. In this case, versions of some sub-sub-dependency are ill-defined. For instance That's all for a source-based package. I also tried 0.3.8-3 as a binary package and comes very close to working, so we might not need all the hair-tearing mentioned above. |
5abf82b
to
f425aa3
Compare
Alright, finally got everything worked out. This doesn't build everything from source, like it would do when I started this PR, but it uses the distributed package from Popcorntime and combines that with nwjs from NixOS. This way, things work. 🎉 Also, Popcorntime depends on nwjs 0.12, so I've added that as a separate commit. Let me know if I should squash these 2 before it can be merged. I've also removed the fromCi stuff, since that is not needed anymore. @domenkozar Yes, it's ready now. |
@bobvanderlinden: Sweet! Thanks! |
Pushed to master (e6e3384). |
And cherry-picked to release-15.09 (0903632). Closing. |
@bjornfor Thanks for the quick merge |
@bobvanderlinden Thanks for all the work you have done. |
@rnhmjoj Not a packaging issue. See #8800 (comment) Try a recent episode. For instance Adventure Time S06E16 works flawlessly. |
@bobvanderlinden Thank you again. |
This upgrades popcorntime from 0.3.7.2 to 0.3.8-0. 0.3.8 was packaged differently, which required to build the package from source. It also needed nwjs 0.12, which is also added in this PR.
Many thanks to @MasseGuillaume who did the initial work for the upgrade. This should solve issue #8800.