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

turtlevcs: init at 0.6 #257062

Closed
wants to merge 2 commits into from
Closed

turtlevcs: init at 0.6 #257062

wants to merge 2 commits into from

Conversation

BurNiinTRee
Copy link
Contributor

Closes #252317

Adds a turtle package.
The nautilus plugin requires gnome.nautilus-python to be installed separately.

The package is a bit weird, since turtle_cli subcommands are implemented through subprocesses
of either $PYTHON or sys.executable. This doesn't work well with buildPythonApplications hooks
for setting up python dependencies.

I'm not very familiar with neither python packaging in general, nor nixpkgs' infrastructure for it,
so there might be better ways of achieving this.

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@FliegendeWurst
Copy link
Member

The package needs a meta attribute.

Copy link
Contributor

@benneti benneti left a comment

Choose a reason for hiding this comment

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

Is there a reason you choose the by name category? A similar program (gitkraken) is in pkgs/applications/version-management/ so this might be better suited there as well?
Additionally, I think you should also put a call to the program in pkgs/top-level/all-packages.nix.
After this it would also be much easier for me to test the PR

pkgs/by-name/tu/turtle/package.nix Outdated Show resolved Hide resolved
@FliegendeWurst
Copy link
Member

FliegendeWurst commented Nov 2, 2023

Is there a reason you choose the by name category?

It is recommended in the documentation (since #237439 was merged). When using the pkgs/by-name hierarchy there is no need to modify all-packages.nix.

@benneti
Copy link
Contributor

benneti commented Nov 2, 2023

oops, thanks for the heads up!

@BurNiinTRee
Copy link
Contributor Author

I removed the trailing comma and set $PYTHONPATH in the wrapper to get rid of the extra derivation.
Thanks for the suggetsion

Copy link
Contributor

@benneti benneti left a comment

Choose a reason for hiding this comment

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

Great, thank you I think it looks much nicer now!
I think before someone merges this you should sqush to two commits, one adding you as the maintainer and one for the init of the package.

@BurNiinTRee
Copy link
Contributor Author

Done

@bryango
Copy link
Member

bryango commented Feb 16, 2024

Wow, this is nice! Although it has been sitting for a while... Let me see if ofborg complains...
Update: confirmed ofborg happy, I am also not a committer, will try to advertise in discourse.

ofborg eval

@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/1452

preFixup = ''
makeWrapperArgs+=("''${gappsWrapperArgs[@]}")
# It can't find pygit2 without also adding $PYTHONPATH to the wrapper
makeWrapperArgs+=( --prefix PYTHONPATH : $out/${python3.sitePackages}:$PYTHONPATH )
Copy link
Member

Choose a reason for hiding this comment

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

We cannot use $PYTHONPATH like this as it draws in build time dependencies

rev = version;
hash = "sha256-ShZlGZeIjsMOLrdPsl6WphGZcWPrhTQHv2b7HFVm4do=";
};
format = "setuptools";
Copy link
Member

Choose a reason for hiding this comment

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

Please use the new pyproject = true setting instead of format

python3.pkgs.buildPythonApplication rec {
pname = "turtle";
version = "0.6";
src = fetchFromGitLab {
Copy link
Member

Choose a reason for hiding this comment

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

A new line here and there similar to other packages would help readability a lot.

description = "A graphical interface for version control intended to run on gnome and nautilus";
homepage = "https://gitlab.gnome.org/philippun1/turtle";
license = licenses.gpl3Plus;
maintainers = with maintainers; [ burniintree ];
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
maintainers = with maintainers; [ burniintree ];
mainProgram = "turtle";
maintainers = with maintainers; [ burniintree ];

@Aleksanaa Aleksanaa added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 21, 2024
@Aleksanaa Aleksanaa mentioned this pull request May 22, 2024
13 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.

Package request: turtle (gnome)
8 participants