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.

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

rmapi: init at 0.0.11 #85898

Merged
merged 1 commit into from May 25, 2020
Merged

rmapi: init at 0.0.11 #85898

merged 1 commit into from May 25, 2020

Conversation

NickHu
Copy link
Contributor

@NickHu NickHu commented Apr 23, 2020

Motivation for this change

Add the rMAPI tool.

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.

@doronbehar
Copy link
Contributor

Please switch to vendorSha256, this should fix also the CI error.

@doronbehar
Copy link
Contributor

Oh I see you commented at ryantm/nixpkgs-update#203, I think it'll work out for you if you'd not only rename the variable modSha256 to vendorSha256, but also zero it out before you rerun nix-build.

@doronbehar
Copy link
Contributor

This works:

diff --git i/pkgs/applications/misc/remarkable/rmapi/default.nix w/pkgs/applications/misc/remarkable/rmapi/default.nix
index 66be455a040..0fe0e03060a 100644
--- i/pkgs/applications/misc/remarkable/rmapi/default.nix
+++ w/pkgs/applications/misc/remarkable/rmapi/default.nix
@@ -11,7 +11,7 @@ buildGoModule rec {
     sha256 = "0zks1pcj2s2pqkmw0hhm41vgdhfgj2r6dmvpsagbmf64578ww349";
   };
 
-  modSha256 = "0w2qiafs5gkgv00yz16bx8yis6gnpxbgqliwrhj5k6z8yy9s7b17";
+  vendorSha256 = "077s13pcql5w2m6wzls1q06r7p501kazbwzxgfh6akwza15kb4is";
 
   meta = with stdenv.lib; {
     description = "A Go app that allows access to the ReMarkable Cloud API programmatically";

@doronbehar
Copy link
Contributor

@Enteee, personally, I like this PR much more then yours - it's just so much cleaner and regular. I think I've sort of understood now what was your motivation behind the IFD thing, it's just that I didn't catch initially that you tried to push Nix support upstream at juruen/rmapi#78 .

Never the less, I'm still a bit puzzled - almost every Go app that we ship doesn't include default.nix, shell.nix and all of those files, and most people seem happy about it. Even packages which do have these files and naturally are maintained both upstream and downstream by Nixpkgs contributors, are not packaged using IFD hooks such as you want to apply in #74657. Examples: https://github.com/NixOS/nixpkgs/search?q=maintainers.mic92&unscoped_q=maintainers.mic92

I think you'll like the yet unstable feature flakes - NixOS/rfcs#49 . When it'll stabilized, it'll be possible to do cool stuff like you tried in #74657 with less workarounds.

@NickHu
Copy link
Contributor Author

NickHu commented May 25, 2020

@doronbehar Oh nice it works now. I wonder why it gave me an empty output before (even though the build log appeared to get all the dependencies and actually compile it). Nonetheless I've updated it so i think it ought to be good to go

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Passes build and appears to be running as should.

Copy link
Contributor

@Enteee Enteee left a comment

Choose a reason for hiding this comment

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

looks good

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/163

@Lassulus Lassulus merged commit ab3f7b6 into NixOS:master May 25, 2020
@Enteee Enteee mentioned this pull request May 26, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants