pmp-library: init at 3.0.0#405747
Conversation
bd96d87 to
691e1dc
Compare
f866bde to
fb6cd52
Compare
GirardR1006
left a comment
There was a problem hiding this comment.
Not a CMake expert here. I checked that the package builds, does not add other dependencies and ran nixpkg-review for completeness.
There was a problem hiding this comment.
Recs made:
- Reorganize parameter list, placing update script and overridable attr at the bottom.
- Revise patch comment to be more clear about purpose.
- Use
lib.cmakeFeatureinstead of string literal. - Replace
doCheckwith check for cross-comp (this is standard instead ofdoCheck = true;) - Reorganize meta attrs.
- Add
meta.longDescription.
diff --git a/pkgs/by-name/pm/pmp-library/package.nix b/pkgs/by-name/pm/pmp-library/package.nix
index d70334dd3738..de3abbc6bd46 100644
--- a/pkgs/by-name/pm/pmp-library/package.nix
+++ b/pkgs/by-name/pm/pmp-library/package.nix
@@ -5,9 +5,6 @@
cmake,
eigen,
gtest,
- nix-update-script,
-
- withViewer ? false,
glew,
glfw,
libGL,
@@ -16,8 +13,9 @@
libXi,
libXinerama,
libXrandr,
+ nix-update-script,
+ withViewer ? false,
}:
-
stdenv.mkDerivation (finalAttrs: {
pname = "pmp-library";
version = "3.0.0";
@@ -35,7 +33,8 @@ stdenv.mkDerivation (finalAttrs: {
];
patches = [
- # use embedded imgui since it's not easy to patch
+ # Vendor in nixpkgs libraries for `glew` and `glfw`, imgui is
+ # unvendored due to a build failure
./cmake_find_package.patch
];
@@ -62,22 +61,31 @@ stdenv.mkDerivation (finalAttrs: {
(lib.cmakeBool "PMP_BUILD_DOCS" false)
(lib.cmakeBool "PMP_BUILD_EXAMPLES" false)
(lib.cmakeBool "PMP_BUILD_VIS" withViewer)
- "-DEIGEN3_INCLUDE_DIR=${eigen}/include/eigen3"
+ (lib.cmakeFeature "EIGEN3_INCLUDE_DIR" "${eigen}/include/eigen3")
];
- doCheck = true;
- nativeCheckInputs = [
- gtest
- ];
+ doCheck = stdenv.buildPlatform.canExecute stdenv.hostPlatform;
+
+ nativeCheckInputs = [ gtest ];
passthru.updateScript = nix-update-script { };
meta = {
- homepage = "https://www.pmp-library.org";
description = "Polygon Mesh Processing Library";
+ longDescription = ''
+ The Polygon Mesh Processing Library is a modern C++ open-source
+ library for processing and visualizing polygon surface meshes. Its
+ main features are:
+
+ - An efficient and easy-to-use mesh data structure
+ - Standard algorithms such as decimation, remeshing, subdivision, or smoothing
+ - Ready-to-use visualization tools
+ - Seamless cross-compilation to JavaScript
+ '';
+ homepage = "https://www.pmp-library.org";
changelog = "https://github.com/pmp-library/pmp-library/blob/${finalAttrs.version}/CHANGELOG.md";
- platforms = lib.platforms.all;
license = lib.licenses.mit;
maintainers = with lib.maintainers; [ yzx9 ];
+ platforms = lib.platforms.all;
};
})
ghost
left a comment
There was a problem hiding this comment.
Review 2
- Revise description.
diff --git a/pkgs/by-name/pm/pmp-library/package.nix b/pkgs/by-name/pm/pmp-library/package.nix
index 2f8a95c038fd..1a7a5bc10202 100644
--- a/pkgs/by-name/pm/pmp-library/package.nix
+++ b/pkgs/by-name/pm/pmp-library/package.nix
@@ -72,7 +72,7 @@ stdenv.mkDerivation (finalAttrs: {
passthru.updateScript = nix-update-script { };
meta = {
- description = "Polygon Mesh Processing Library";
+ description = "Library for processing and visualizing polygon surface meshes";
longDescription = ''
The Polygon Mesh Processing Library is a modern C++ open-source
library for processing and visualizing polygon surface meshes.|
Thanks @normalcea for your detailed review! I’d prefer to keep the current official description, as it doesn’t violate the nixpkgs contribution conventions and clearly reflects the library’s purpose and name. Also, I noticed that the package uses an MIT-style license, which isn’t currently reflected in nixpkgs. Should I go ahead and update the license field? |
|
Your welcome, Regarding package description as per https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#meta-attributes
"Polygon Mesh Processing Library" is the expanded acronym of the package's project name ( "Library for processing and visualizing polygon surface meshes" is better for describing the package itself. "C++ Library for processing and visualizing polygon surface meshes" would also work as well. This is also taken from the project homepage (https://www.pmp-library.org/)
Agh, well that's not good. I think we have to contact upstream in this case since this is a legal issue now. If you wish to open up an issue in upstream's GitHub issue tracker that's up to you since this possibly blocks inclusion into nixpkgs until this is settled. Otherwise someone in nixpkgs legal should look at this since I'm not a lawyer. |
25b8cc3 to
5f70fb1
Compare
|
I have updated the |
There was a problem hiding this comment.
- Opened an issue with them to see if they'll just revert to using a plain old MIT license.
- Removed whitespace between meta attrs and comment
- Removed
letbinding for simplicity. - This license was introduced in commit pmp-library/pmp-library@5137606 which was in 2019 and they have a custom spdxId as well so I added that.
diff --git a/pkgs/by-name/pm/pmp-library/package.nix b/pkgs/by-name/pm/pmp-library/package.nix
index beb9c14b9fdf..5096a71d6935 100644
--- a/pkgs/by-name/pm/pmp-library/package.nix
+++ b/pkgs/by-name/pm/pmp-library/package.nix
@@ -85,20 +85,16 @@ stdenv.mkDerivation (finalAttrs: {
'';
homepage = "https://www.pmp-library.org";
changelog = "https://github.com/pmp-library/pmp-library/blob/${finalAttrs.version}/CHANGELOG.md";
-
# Upstream uses a MIT-style license with an explicit employer disclaimer
# to clarify that the contributor’s employer bears no responsibility.
- license =
- let
- name = "MIT License with employer disclaimer";
- in
- {
- shortName = name;
- fullName = name;
- free = true;
- redistributable = true;
- url = "https://github.com/pmp-library/pmp-library/blob/${finalAttrs.version}/LICENSE.txt";
- };
+ license = {
+ shortName = "MIT License with employer disclaimer";
+ fullName = "MIT License with employer disclaimer";
+ spdxId = "MIT-with-employer-disclaimer";
+ free = true;
+ redistributable = true;
+ url = "https://github.com/pmp-library/pmp-library/blob/${finalAttrs.version}/LICENSE.txt";
+ };
maintainers = with lib.maintainers; [ yzx9 ];
platforms = lib.platforms.all;
};
I didn't realize that lib.mkLicense isn't exposed in nixpkgs and lives entirely in lib/licenses.nix the more you know.
|
Many thanks for your help! I’ve followed your suggestions, except for adding spdxId, since the custom license doesn’t exist in the SPDX License list, and the field isn’t required in nixpkgs. |
|
|
Hi @normalcea, I noticed that this package is currently broken on Darwin. I'm still willing to fix it if it's acceptable to keep it, or we could mark it as unfree instead. Otherwise, I'll close this PR. |
|
Darwin support is not required for a package to be included (though I think this PR should be retired for the time being until a package arrives in-tree that uses this library. There hasn't been a new upstream release in 2 years as well. The licensing issue isn't a dealbreaker since the disclaimer listed doesn't add any additional clauses to the MIT license since all liability is voided anyway. So it has nothing to do with that thankfully (though it is still very annoying). |
|
@normalcea Thanks for the review! That makes sense. I’ll close this PR since I’ve actually forgotten why I added this package in the first place. If anyone needs this package, please let me know. |
Homepage: https://www.pmp-library.org
Code: https://github.com/pmp-library/pmp-library
The Polygon Mesh Processing Library is a modern C++ open-source library for processing and visualizing polygon surface meshes. Its main features are:
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.