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

surfer: init at v0.2.0 #324637

Merged
merged 2 commits into from
Sep 11, 2024
Merged

surfer: init at v0.2.0 #324637

merged 2 commits into from
Sep 11, 2024

Conversation

hakan-demirli
Copy link
Member

@hakan-demirli hakan-demirli commented Jul 4, 2024

Description of changes

Supersedes #320993

Closes #299458.
@Guanran928

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.

@hakan-demirli

This comment was marked as outdated.

}:
rustPlatform.buildRustPackage rec {
pname = "surfer";
version = "v0.2.0";
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
version = "v0.2.0";
version = "0.2.0";

from the contributing guidelines

There are a few naming guidelines:

  • The pname attribute should be identical to the upstream package name.

  • The pname and the version attribute must not contain uppercase letters — e.g., "mplayer" instead of "MPlayer".

  • The version attribute must start with a digit e.g., "0.3.1rc2".

Copy link
Member

Choose a reason for hiding this comment

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

Remember to edit commit message accordingly

pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
@hakan-demirli
Copy link
Member Author

hakan-demirli commented Jul 5, 2024

Thanks for the review. My takeaways are. I should have:

  • Used nixfmt-rfc-style not alejandra.
  • Read the docs not skim.

I will open another separate PR to add myself as a maintainer. Then I will fix this PR.

@getchoo
Copy link
Member

getchoo commented Jul 5, 2024

you don't need to open another PR to add yourself as a maintainer; you only need a separate commit in this one

@hakan-demirli

This comment was marked as outdated.

Copy link
Contributor

@Guanran928 Guanran928 left a comment

Choose a reason for hiding this comment

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

Rewording the first commit would be nice,
image

Copy link
Contributor

@Guanran928 Guanran928 left a comment

Choose a reason for hiding this comment

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

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

1 package built:
  • surfer

pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/su/surfer/package.nix Outdated Show resolved Hide resolved
@kacper-uminski
Copy link

Contributor to surfer here. It would be most appreciated to have this merged. :) Is there anything left that needs to be done which I could help with?

@bgamari
Copy link
Contributor

bgamari commented Sep 10, 2024

While this appears to build and is largely functional, any attempt to use functions involving the file picker fail with [ERROR] pick_file error No such file or directory (os error 2).

@bgamari
Copy link
Contributor

bgamari commented Sep 10, 2024

While this appears to build and is largely functional, any attempt to use functions involving the file picker fail with [ERROR] pick_file error No such file or directory (os error 2).

The reason for this appears to be that the package somehow depends upon nixpkgs#gnome3.zenity for file selection. nix shell nixpkgs#gnome3.zenity github:hakan-demirli/nixpkgs/surfer#surfer -c surfer works as expected.

@Aleksanaa
Copy link
Member

The reason for this appears to be that the package somehow depends upon nixpkgs#gnome3.zenity for file selection. nix shell nixpkgs#gnome3.zenity github:hakan-demirli/nixpkgs/surfer#surfer -c surfer works as expected.

This is because it's using https://github.com/PolyMeilex/rfd/blob/e0e725ec9a426acf7c93d9877c41e230fcee3fac/src/backend/linux/zenity.rs to open file picker. We can just wrap it with zenity.

@Aleksanaa Aleksanaa merged commit d07d2f9 into NixOS:master Sep 11, 2024
25 of 27 checks passed
@hakan-demirli hakan-demirli deleted the surfer branch September 11, 2024 14:37
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.

Package request: surfer
8 participants