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

sss_code: init at 0.2.0 #302495

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

sss_code: init at 0.2.0 #302495

wants to merge 2 commits into from

Conversation

krovuxdev
Copy link
Contributor

@krovuxdev krovuxdev commented Apr 8, 2024

Description of changes

sss is a command tools for screenshots and code snippets.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@krovuxdev krovuxdev changed the title Sss/v0.1.8 sss: init at 0.1.8 Apr 8, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Apr 8, 2024
Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

  1. Please move to pkgs/by-name. Other directories needs top-level reference or package sets reference.
  2. Looks like you are contributing for the first time. Please add your name to maintainers-maintainer-list.nix following its examples.
  3. It's recommended to leave some blank lines between attributes to improve readability. You can refer to how other packages do it.

pkgs/tools/misc/sss/default.nix Outdated Show resolved Hide resolved
pkgs/tools/misc/sss/default.nix Outdated Show resolved Hide resolved
@krovuxdev
Copy link
Contributor Author

Hello @Aleksanaa :)
I've addressed the three points mentioned earlier. Here's where we stand:

  • Moved to pkgs/by-name: Moved the package to pkgs/by-name as requested.
  • Added name to maintainers-maintainer-list.nix: Added my name following the examples provided since it's my first
  • Added whitespace between attributes: Updated the code to include whitespace between attributes for improved readability.

Please review these changes, and let me know if there's anything else I need to address.

Thanks you

@krovuxdev krovuxdev requested a review from Aleksanaa April 8, 2024 16:02
@Aleksanaa Aleksanaa removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Apr 8, 2024
Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

Please squash all your commits leaving only two:

  • maintainers: add krovuxdev
  • sss: init at 0.1.8

pkgs/by-name/ss/sss/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ss/sss/package.nix Outdated Show resolved Hide resolved
@krovuxdev krovuxdev force-pushed the sss/v0.1.8 branch 2 times, most recently from babd5c0 to b003a4d Compare April 8, 2024 17:54
@krovuxdev krovuxdev requested a review from Aleksanaa April 8, 2024 17:54
@krovuxdev
Copy link
Contributor Author

Hi @Aleksanaa, could you please review the code to see if it's okay now?

@Aleksanaa
Copy link
Member

Please squash all your commits leaving only two:

  • maintainers: add krovuxdev
  • sss: init at 0.1.8

@krovuxdev
Copy link
Contributor Author

@Aleksanaa Hello, thank you for bringing it to my attention. Could you please review my commits to see if they are okay?

@Aleksanaa
Copy link
Member

The maintainer addition commit message is specified in https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions.

@krovuxdev
Copy link
Contributor Author

The maintainer addition commit message is specified in https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#commit-conventions.

Ok, thank you, and now please, review my commits to see if they are correct

@Aleksanaa
Copy link
Member

@ofborg eval

src = fetchFromGitHub {
owner = "SergioRibera";
repo = "sss";
rev = "sss_code/v{version}";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rev = "sss_code/v{version}";
rev = "sss_code/v${version}";

pkgs/by-name/ss/sss/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ss/sss/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ss/sss/package.nix Outdated Show resolved Hide resolved
@Aleksanaa
Copy link
Member

Also your Cargo.Lock is different from the file in the 0.1.8 tag of the repo, but the same with one in the main branch. Seems to be a mistake.

@krovuxdev
Copy link
Contributor Author

Also your Cargo.Lock is different from the file in the 0.1.8 tag of the repo, but the same with one in the main branch. Seems to be a mistake.

@Aleksanaa, yes, that's why I'm using "main" instead of "sss_code/v0.1.8". As you mentioned, the "sss_code/v0.1.8" is quite old. Additionally, a friend advised me to use "main" because it represents the latest version. Remember, you advised me to change the rev to "sss_code" in package.nix.

@Aleksanaa
Copy link
Member

@Aleksanaa, yes, that's why I'm using "main" instead of "sss_code/v0.1.8". As you mentioned, the "sss_code/v0.1.8" is quite old. Additionally, a friend advised me to use "main" because it represents the latest version. Remember, you advised me to change the rev to "sss_code" in package.nix.

Yes, but main is not 0.1.8 as you wrote in the commit message. It is 0.1.8-unstable-2024-03-30. And three months is not a long time interval. If you encounter a bug or build failure, you should use patches to backport a small part of the commit from main branch. Also using Cargo.Lock that is inconsistent with the author's source code will increase the probability of hidden bugs.

@krovuxdev
Copy link
Contributor Author

@Aleksanaa, yes, that's why I'm using "main" instead of "sss_code/v0.1.8". As you mentioned, the "sss_code/v0.1.8" is quite old. Additionally, a friend advised me to use "main" because it represents the latest version. Remember, you advised me to change the rev to "sss_code" in package.nix.

Yes, but main is not 0.1.8 as you wrote in the commit message. It is 0.1.8-unstable-2024-03-30. And three months is not a long time interval. If you encounter a bug or build failure, you should use patches to backport a small part of the commit from main branch. Also using Cargo.Lock that is inconsistent with the author's source code will increase the probability of hidden bugs.

@Aleksanaa, if I set the version as "sss-unstable-2024-03-29" or "unstable-2024-03-30", is that sufficient?

@Aleksanaa
Copy link
Member

No. It doesn't fit the versioning guidelines in https://github.com/NixOS/nixpkgs/tree/master/pkgs#package-naming. And although there is currently no policy whether to uses the -git version, a project that was released three months ago (and this release is a valid) is probably not within the suitable scope.

@krovuxdev krovuxdev changed the title sss: init at 0.1.8 sss_code: init at 0.1.9 Apr 10, 2024
@krovuxdev
Copy link
Contributor Author

I've updated the version of sss_code and also changed the title of my PR. I'll create another PR for sss_cli shortly. Does that sound better?

};

postInstall = ''
rm -f $out/bin/sss_cli
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment here why deleting this.

@krovuxdev
Copy link
Contributor Author

Wait! xd

@krovuxdev
Copy link
Contributor Author

Done!

@krovuxdev
Copy link
Contributor Author

@ofborg eval

@Aleksanaa
Copy link
Member

@ofborg build

@Aleksanaa
Copy link
Member

@ofborg build sss_code

@Aleksanaa
Copy link
Member

error: builder for '/nix/store/i0kfrxx1cpikb5rlcl7p1262vgyvr4dr-crate-serde_repr-0.1.19.tar.gz.drv' failed with exit code 1;
       last 7 log lines:
       >
       > trying https://crates.io/api/v1/crates/serde_repr/0.1.19/download
       >   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
       >                                  Dload  Upload   Total   Spent    Left  Speed
       >   0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
       > curl: (35) Send failure: Connection reset by peer
       > error: cannot download crate-serde_repr-0.1.19.tar.gz from any mirror
       For full logs, run 'nix log /nix/store/i0kfrxx1cpikb5rlcl7p1262vgyvr4dr-crate-serde_repr-0.1.19.tar.gz.drv'.
error: builder for '/nix/store/65pz36pyb76bkvz56bnpdph1z5swdd0y-crate-tauri-winrt-notification-0.2.0.tar.gz.drv' failed with exit code 1;
       last 7 log lines:
       >
       > trying https://crates.io/api/v1/crates/tauri-winrt-notification/0.2.0/download
       >   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
       >                                  Dload  Upload   Total   Spent    Left  Speed
       >   0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
       > curl: (35) Send failure: Connection reset by peer
       > error: cannot download crate-tauri-winrt-notification-0.2.0.tar.gz from any mirror
       For full logs, run 'nix log /nix/store/65pz36pyb76bkvz56bnpdph1z5swdd0y-crate-tauri-winrt-notification-0.2.0.tar.gz.drv'.

@ofborg build sss_code

@krovuxdev
Copy link
Contributor Author

@Aleksanaa And why this error of tauri?
sss_code does not use tauri

@krovuxdev
Copy link
Contributor Author

ahhh now I see, there is a tauri in cargo.lock. ok

buildInputs = [ fontconfig freetype libxcb ];

meta = with lib; {
description = "A CLI/Lib to take amazing screenshot of code";
Copy link
Member

Choose a reason for hiding this comment

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

According to the recommendations for meta attributes description should not start with an article and avoid subjective language, hence I propose to use the following shortened description from the projects readme:

Suggested change
description = "A CLI/Lib to take amazing screenshot of code";
description = "Libraries and tools for building screenshots in a high-performance image format";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afh it's ready. Thank you, I changed the description and also made changes to the code because the package was updated.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 4, 2024
@krovuxdev krovuxdev changed the title sss_code: init at 0.1.9 sss_code: init at 0.2.0 Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants