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

paperlib: init at 3.1.6 #323958

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Conversation

ByteSudoer
Copy link
Contributor

Description of changes

Project description
An open-source academic paper management tool.
Metadata

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Contributor

@ShamrockLee ShamrockLee left a comment

Choose a reason for hiding this comment

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

Given that it's an open-source project, I suggest renaming this package to paperlib-bin and leaving space for the source-built package available for all platforms. name = "paperlib"; should be kept unchanged.

pkgs/by-name/pa/paperlib/package.nix Show resolved Hide resolved
pkgs/by-name/pa/paperlib/package.nix Show resolved Hide resolved
@jackyliu16
Copy link
Contributor

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

1 package built:
  • paperlib

Copy link
Member

@nevivurn nevivurn left a comment

Choose a reason for hiding this comment

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

Also, why were ShamrockLee's reviews marked as resolved?

pkgs/by-name/pa/paperlib/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib/package.nix Outdated Show resolved Hide resolved
@ShamrockLee
Copy link
Contributor

Thank you for addressing the formatting review.

I would still like to hear what you think about my two other reviews (renaming this package to paperlib-bin and exposing binary packages for all systems via passthru). It is confusing to see them marked "resolved" with no changes or comments added.

As you are, I'm a contributor to Nixpkgs without commit access. Even though we cannot merge pull requests or request "blocking changes" during reviews, I still hope to move pull requests and the Nixpkgs project forward by participating in discussions during pull request reviews.

@ByteSudoer
Copy link
Contributor Author

@ShamrockLee, yes I will address them as soon as I can and I re-request your review. Sorry they completely flew over my head.

Copy link
Member

@nevivurn nevivurn left a comment

Choose a reason for hiding this comment

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

Final, minor comments from me, I think.

pkgs/by-name/pa/paperlib/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib/package.nix Outdated Show resolved Hide resolved
@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/1815

@JohnRTitor
Copy link
Contributor

JohnRTitor commented Jul 15, 2024

Fails to build at the moment.

┃        >
┃        > trying https://github.com/Future-Scholars/paperlib-bin/releases/download/release-electron-3.1.6/Paperlib_3.1.6.AppImage
┃        >   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
┃        >                                  Dload  Upload   Total   Spent    Left  Speed
┃        >   0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
┃        > curl: (22) The requested URL returned error: 404
┃        > error: cannot download Paperlib_3.1.6.AppImage from any mirror
┃        For full logs, run 'nix log /nix/store/y3yqsmfz4f6ny2d4k9ngrbnp64cwyl5w-Paperlib_3.1.6.AppImage.drv'.
┣━ Dependency Graph:

@JohnRTitor
Copy link
Contributor

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

1 package failed to build:
  • paperlib

@ByteSudoer
Copy link
Contributor Author

@JohnRTitor Changing the pname variable messed up the upstream url(where the src would be fetched from). Sorry about that. It would be fixed soon.

Copy link
Contributor

@JohnRTitor JohnRTitor left a comment

Choose a reason for hiding this comment

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

In addition, the whole file should be formatted with nixfmt-rfc-style

pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/pa/paperlib-bin/package.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

I would still like to hear what you think about my two other reviews (renaming this package to paperlib-bin and exposing binary packages for all systems via passthru). It is confusing to see them marked "resolved" with no changes or comments added.

That is not what we are generally doing. This is usually reflected by meta.sourceProvonance and we only have the -bin suffix if there are two packages.

@SuperSandro2000 SuperSandro2000 merged commit df4355e into NixOS:master Jul 16, 2024
24 of 26 checks passed
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.

9 participants