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

omorfi: init at 0.9.9 #238378

Merged
merged 5 commits into from
Jul 26, 2023
Merged

omorfi: init at 0.9.9 #238378

merged 5 commits into from
Jul 26, 2023

Conversation

Lurkki14
Copy link
Member

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.

@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 18, 2023
@Lurkki14 Lurkki14 force-pushed the lurkki/omorfi branch 2 times, most recently from a4c4d99 to dca50e6 Compare June 18, 2023 08:31
@sikmir
Copy link
Member

sikmir commented Jun 18, 2023

Could you please split into separate commits (libvoiko, hfst, omorfi, etc)?

@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Jun 19, 2023
@Lurkki14 Lurkki14 requested a review from sikmir June 19, 2023 15:06
stdenv.mkDerivation rec {
pname = "hfst-ospell";
version = "0.5.3";
buildInputs = [
Copy link
Member

Choose a reason for hiding this comment

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

Nit: a bit unusual order, more common is pname, version, src, nativeBuildInputs, buildInputs, meta.

@Lurkki14 Lurkki14 requested a review from sikmir June 20, 2023 16:39
@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-ready-for-review/3032/2393

@Lurkki14 Lurkki14 requested a review from sikmir July 23, 2023 17:10
@Lurkki14 Lurkki14 requested a review from sikmir July 24, 2023 09:46
homepage = "https://github.com/flammie/omorfi";
license = licenses.gpl3;
maintainers = with maintainers; [ lurkki ];
# Ofborg build error
Copy link
Member

Choose a reason for hiding this comment

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

This comment isn't helpful for other to pick this up

Copy link
Member Author

@Lurkki14 Lurkki14 Jul 25, 2023

Choose a reason for hiding this comment

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

Not sure I can see easily anywhere what the error is, since I don't have a Darwin machine

hfst
];

# Fixes some improper import paths
Copy link
Member

Choose a reason for hiding this comment

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

So we can run a pythonImportsCheck?

Can/Should we run https://github.com/flammie/omorfi/tree/main/test ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those tests are already runnable in the checkPhase of omorfi

homepage = "https://github.com/flammie/omorfi";
license = licenses.gpl3;
maintainers = with maintainers; [ lurkki ];
# Ofborg build error
Copy link
Member

Choose a reason for hiding this comment

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

not to helpful for others to pick this up and fix it

Copy link
Member

Choose a reason for hiding this comment

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

omorfi> checking python module: libhfst... no
omorfi> configure: error: failed to find required module libhfst

Looks like it should be easy to fix. I'll try to investigate...

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
# Ofborg build error
# fails to find libhfst on darwin

pkg-config
python3
zip
python3Packages.wrapPython
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
python3Packages.wrapPython
python3.pkgs.wrapPython

];

buildInputs = [
python3Packages.hfst
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
python3Packages.hfst
python3.pkgs.hfst

Comment on lines 33 to 34
# Package builds but tests fail on Darwin
doCheck = !stdenv.isDarwin;
Copy link
Member

Choose a reason for hiding this comment

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

I've figured out how to fix tests on darwin:

Suggested change
# Package builds but tests fail on Darwin
doCheck = !stdenv.isDarwin;
preCheck = lib.optionalString stdenv.isDarwin ''
export DYLD_LIBRARY_PATH="${foma}/lib"
'';

hash = "sha256-0MIQ54dCxyAfdgYWmmTVF+Yfa15K2sjJyP1JNxwHP2M=";
};

sourceRoot = "source/libvoikko";
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
sourceRoot = "source/libvoikko";
sourceRoot = "${src.name}/libvoikko";

pname = "hfst";
inherit (pkgs.hfst) version src;

sourceRoot = "source/python";
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
sourceRoot = "source/python";
sourceRoot = "${src.name}/python";

pname = "omorfi";
inherit (pkgs.omorfi) src version;

sourceRoot = "source/src/python";
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
sourceRoot = "source/src/python";
sourceRoot = "${src.name}/src/python";

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi !

Thanks for your first contribution.

I added some feedback. Most of it is to use the finalAttrs pattern.

Feel free to let me know if you need some assistance.

Thanks!

, pkg-config
}:

stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the finalAttrs pattern here.

Since this is your first PR, let me share a couple of useful links regarding this:

  1. History: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293
  2. Documentation: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes
  3. Recent example of implementation: https://github.com/NixOS/nixpkgs/compare/17f96f7b978e61576cfe16136eb418f74fab9952..9e6ea843e473d34d4f379b8b0d8ef0425a06defe

, zlib
}:

stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the finalAttrs pattern here.

Since this is your first PR, let me share a couple of useful links regarding this:

  1. History: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293
  2. Documentation: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes
  3. Recent example of implementation: https://github.com/NixOS/nixpkgs/compare/17f96f7b978e61576cfe16136eb418f74fab9952..9e6ea843e473d34d4f379b8b0d8ef0425a06defe

, python3
}:

stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

, zip
}:

stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@drupol drupol merged commit 3ea8f62 into NixOS:master Jul 26, 2023
sikmir added a commit to sikmir/nur-packages that referenced this pull request Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants