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

pythonPackages.promnesia: init at 1.1.20230129 #215228

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

qbit
Copy link
Contributor

@qbit qbit commented Feb 8, 2023

This add promnesia and its dependencies.

There are a few things I am not sure about:

  • HPI should be installing $out/bin/hpi, but it doesn't seem to be doing so.
  • promnesia runs a config.py configuration that users create / modify. Currently, that config.py fails to load HPI (lib is named my) and the promnesia lib when commands like promnesia doctor are used.

CC @mweinelt


  • 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.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@qbit qbit requested review from FRidh and jonringer as code owners February 8, 2023 01:38
@qbit qbit marked this pull request as draft February 8, 2023 01:38
@qbit qbit changed the title Draft: pythonPackages.promnesia: init at 1.1.20230129 pythonPackages.promnesia: init at 1.1.20230129 Feb 8, 2023
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 8, 2023
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

First pass. A few changes that are required on all packages:

  • We require running tests, when they're available
    • if not available on pypi, we fetch from the git source
  • We require explicit nativeCheckInputs, usually pytestCheckHook or unittestCheckHook
  • We require a format set just below version
    • pyproject.toml? then pyproject
    • setup.py? setuptools
  • The fetcher should use hash, not the legacy sha256 attribute

@@ -0,0 +1,25 @@
{ buildPythonPackage, fetchPypi, lib, appdirs, sqlalchemy, setuptools-scm }:
Copy link
Member

Choose a reason for hiding this comment

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

One argument per line, please.


buildPythonPackage rec {
pname = "cachew";
version = "0.11.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 = "0.11.0";
version = "0.11.0";
format = "setuptools";

We set an explicit format for python packages. This package uses a setup.py, so it is setuptools.

Comment on lines 13 to 16
src = fetchPypi {
inherit pname version;
sha256 = "sha256-4qjgvffInKRpKST9xbwwC2+m8h3ups0ZePyJLUU+KhA=";
};
Copy link
Member

Choose a reason for hiding this comment

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

Move the src below version and format.

Also did you notice that the pypi sdist does not contain tests? We should fetch from GitHub I think.

cachew> running test
cachew> WARNING: Testing via this command is deprecated and will be removed in a future version. Users looking for a generic test entry point independent of test runner are encouraged to use tox.
cachew> running egg_info
cachew> writing src/cachew.egg-info/PKG-INFO
cachew> writing dependency_links to src/cachew.egg-info/dependency_links.txt
cachew> writing requirements to src/cachew.egg-info/requires.txt
cachew> writing top-level names to src/cachew.egg-info/top_level.txt
cachew> reading manifest file 'src/cachew.egg-info/SOURCES.txt'
cachew> adding license file 'LICENSE.txt'
cachew> writing manifest file 'src/cachew.egg-info/SOURCES.txt'
cachew> running build_ext
cachew> 
cachew> ----------------------------------------------------------------------
cachew> Ran 0 tests in 0.000s
cachew> 
cachew> OK
cachew> Finished executing setuptoolsCheckPhase

Also tests are run implicitly, which is deprecated. The project uses pytest, and we can use pytestCheckHook in checkInputs after updating the src.

}:

buildPythonPackage rec {
pname = "HPI";
Copy link
Member

Choose a reason for hiding this comment

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

The pname and attribute need to be normalized according to PEP0503. In this case that means lowercasing them.

sha256 = "sha256-cozMmfBF7D1qCZFjf48wRQaeN4MhdHAAxS8tGp/krK8=";
};

nativeBuildInputs = [ setuptools-scm ];
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
nativeBuildInputs = [ setuptools-scm ];
nativeBuildInputs = [
setuptools-scm
];

In python-modules we do expand all lists.


meta = with lib; {
homepage = "https://github.com/karlicoss/promnesia";
description = "Another piece of your extended mind";
Copy link
Member

Choose a reason for hiding this comment

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

While that description is what upstream is offering it is rather meaningless.

}:

buildPythonPackage rec {
pname = "promnesia";
Copy link
Member

Choose a reason for hiding this comment

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

Is this a library or an application? We only keep libraries in python-modules. Application need to be moved elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's both as far as I can tell - not sure how that changes the rules (if at all).. There is the command promnesia, but the configuration uses it as a library: https://github.com/karlicoss/promnesia/blob/master/src/promnesia/misc/config_example.py

Maybe it's both?

Comment on lines 38 to 59
propagatedBuildInputs = [
beautifulsoup4
cachew
fastapi
HPI
httptools
logzero
lxml
mistletoe
more-itertools
mypy
orgparse
pytz
setuptools
sqlcipher3
tzlocal
urlextract
uvicorn
uvloop
watchfiles
websockets
];
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Then we also remove the linting dependencies which do not belong to propagatedBuildInputs

@@ -4376,6 +4378,8 @@ self: super: with self; {

hpccm = callPackage ../development/python-modules/hpccm { };

HPI = callPackage ../development/python-modules/HPI { };
Copy link
Member

@mweinelt mweinelt Feb 8, 2023

Choose a reason for hiding this comment

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

Suggested change
HPI = callPackage ../development/python-modules/HPI { };
hpi = callPackage ../development/python-modules/hpi { };

doCheck = true;

meta = with lib; {
description = "";
Copy link
Member

Choose a reason for hiding this comment

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

Missing description

Copy link
Member

Choose a reason for hiding this comment

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

The description that upstream uses is "Emacs org-mode parser in Python":

Suggested change
description = "";
description = "Emacs org-mode parser in Python";

@mweinelt
Copy link
Member

mweinelt commented Feb 8, 2023 via email

Comment on lines 19 to 20
description =
"Transparent and persistent cache/serialization powered by type hints";
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
description =
"Transparent and persistent cache/serialization powered by type hints";
description = "Transparent and persistent cache/serialization powered by type hints";

I am going to set this project asside for a bit..
@tomodachi94
Copy link
Member

@qbit any updates on this?

@qbit
Copy link
Contributor Author

qbit commented May 29, 2023

@qbit any updates on this?

I gave up after being (iirc) 10 new deps deep. There are also some issues upstream(s) are sorting out: pudo/dataset#415

@tomodachi94
Copy link
Member

tomodachi94 commented May 29, 2023

@qbit do you have a list of packages that need to be created? I'm willing to help take care of some of them.

EDIT: Also, there is a way to apply patches. See this file for an example of how to do that.

@qbit
Copy link
Contributor Author

qbit commented May 30, 2023

IIRC the last commit had all of the deps that were needed for functionality - but they are missing the test deps (and probably other test deps)

@tomodachi94
Copy link
Member

tomodachi94 commented May 30, 2023

Ah, okay. This PR might merge a little faster if you split out some of the dependencies into other PRs. There are a few packages in here that don't have any issues and are probably ready for merge.

Feel free to tag me on any PRs related to this.

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 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.

5 participants