Skip to content

Conversation

@tomjnixon
Copy link
Contributor

@tomjnixon tomjnixon commented Apr 19, 2025

opencamlib is an open-source library of CAM (computer aided manufacturing) algorithms. It can be optionally used by FreeCAD to enable some advanced CAM features.

It is included in the FreeCAD appimage releases, is only imported when explicitly enabled, and is quick to build, so this should be a low-impact change.

The only really weird bit about this is the version number:

  • Conda releases say they are version 2023.01.11, in the conda metadata, but contain opencamlib-2022.12.18.dist-info (from an outdated version number in pyproject.toml).
  • pypi releases say they are version 2023.1.11, but also contain opencamlib-2022.12.18.dist-info.

I chose to align the nixpkgs and python package version numbers with the git tag, which is 2023.01.11. Unfortunately this is normalised by python to 2023.1.11, which really shouldn't matter, but is annoying.

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 (checked on x86_64-linux, build tested on aarch64-linux)
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Apr 19, 2025
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 19, 2025
@nix-owners nix-owners bot requested a review from natsukium April 19, 2025 12:37
@tomjnixon tomjnixon marked this pull request as draft April 19, 2025 12:44
@tomjnixon
Copy link
Contributor Author

accidentally based this on an out-of-date master branch -- please stand by

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 19, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 19, 2025
@tomjnixon tomjnixon marked this pull request as ready for review April 19, 2025 16:44
@tomjnixon
Copy link
Contributor Author

sorted, it just took a while to build and test. sorry about the noise.

@qbisi
Copy link
Contributor

qbisi commented Apr 19, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 400063


x86_64-linux

❌ 1 package failed to build:
  • freecad-qt6
✅ 6 packages built:
  • freecad
  • freecad-wayland
  • python312Packages.opencamlib
  • python312Packages.opencamlib.dist
  • python313Packages.opencamlib
  • python313Packages.opencamlib.dist

aarch64-linux

❌ 3 packages failed to build:
  • freecad
  • freecad-qt6
  • freecad-wayland
✅ 4 packages built:
  • python312Packages.opencamlib
  • python312Packages.opencamlib.dist
  • python313Packages.opencamlib
  • python313Packages.opencamlib.dist

x86_64-darwin

❌ 4 packages failed to build:
  • python312Packages.opencamlib
  • python312Packages.opencamlib.dist
  • python313Packages.opencamlib
  • python313Packages.opencamlib.dist

aarch64-darwin

❌ 4 packages failed to build:
  • python312Packages.opencamlib
  • python312Packages.opencamlib.dist
  • python313Packages.opencamlib
  • python313Packages.opencamlib.dist

@tomjnixon
Copy link
Contributor Author

Thanks.

freecad-qt6 builds and tests fine for me on x86_64-linux though -- do you have any logs available? particularly for darwin (which i can't test) and aarch64 (which will take a while to build).

Copy link
Contributor

@qbisi qbisi left a comment

Choose a reason for hiding this comment

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

Nice contribution. Some nit advice are convention related. Support Darwin platform is not mandantory though, since we dont have freecad built on darwin now.

@qbisi qbisi requested a review from NickCao April 19, 2025 21:10
@qbisi
Copy link
Contributor

qbisi commented Apr 19, 2025

@NickCao, does the license of https://github.com/aewallin/opencamlib match lgpl21Plus.

@qbisi
Copy link
Contributor

qbisi commented Apr 19, 2025

Thanks.

freecad-qt6 builds and tests fine for me on x86_64-linux though -- do you have any logs available? particularly for darwin (which i can't test) and aarch64 (which will take a while to build).

I can build freecad-qt6 with this commit on my x86_64-linux computer too. We can safely ignore the failure report on linux platform.
building opencamlib on darwin platform require extra buildInputs llvmPackages.openmp

@qbisi
Copy link
Contributor

qbisi commented Apr 19, 2025

package freecad has some unused buildInputs

nix-check-deps .#freecad-qt6        
/nix/store/j0q8199pgaj42rlpm71pk5kgs0lxj87g-freecad-1.0.0.drv has unused dependency: /nix/store/0zlh3zr46pd850c8bm9f8jgin25sxs8r-swig-4.3.0.drv
/nix/store/j0q8199pgaj42rlpm71pk5kgs0lxj87g-freecad-1.0.0.drv has unused dependency: /nix/store/ydz5x7iiqvc9dy08hn1a7kld3350rcf3-libXmu-1.2.1.drv
/nix/store/j0q8199pgaj42rlpm71pk5kgs0lxj87g-freecad-1.0.0.drv has unused dependency: /nix/store/ps1lxw1lj5icdx8305sv7knrsl6wssv2-libf2c-20160102.drv
/nix/store/j0q8199pgaj42rlpm71pk5kgs0lxj87g-freecad-1.0.0.drv has unused dependency: /nix/store/3gc318i9mlb5c595gvsxzi7fddwi4yi5-doxygen-1.13.2.drv

but it does use opencamlib

@tomjnixon
Copy link
Contributor Author

tomjnixon commented Apr 20, 2025

Thanks for the review, i've made the requested changes. Also fixed the init commit message to better match other python packages.

I picked lgpl21 rather than lgpl21Plus because the readme says:

From August 2018 OpenCAMLib is released under LGPL license.

And includes an LGPL 2.1 sticker.

The LGPL 2.1 text in COPYING says:

Each version is given a distinguishing version number. If the Library
specifies a version number of this License which applies to it and
"any later version", you have the option of following the terms and
conditions either of that version or of any later version published by
the Free Software Foundation. If the Library does not specify a
license version number, you may choose any version ever published by
the Free Software Foundation.

Arguably a version number is specified (in COPYING and the sticker), so as published it's just 2.1.

Curiously the anaconda package specifies LGPL 2.1 or later, since this commit (by a recent opencamlib contributor):

conda-forge/opencamlib-feedstock@3113199

I think it's best to leave it as-is unless it is clarified in the opencamlib repository.

@qbisi
Copy link
Contributor

qbisi commented Apr 20, 2025

nixpkgs is migrating ambiguos (l)gpl* license to either (l)gplOnly or (l)gplPlus license.
So choose either lgpl21Only or lgpl21Plus.

Does this line declare opencamlib is published under the lgpl21 or later license
https://github.com/aewallin/opencamlib/blob/95b036fe28ce6d77c97b98e5fbc337904ae49560/COPYING#L474

@tomjnixon
Copy link
Contributor Author

Just found the debian package metadata, which marks it as LGPL 2.1 plus, so let's go with that:

https://github.com/aewallin/opencamlib/blob/2023.01.11/src/deb/debian_copyright.txt

I'll fix it tomorow.

Here is a list of exectable under $out/bin

This looks like the output of a different package; i guess it was intended for #400133.

@tomjnixon
Copy link
Contributor Author

done

Copy link
Contributor

@qbisi qbisi left a comment

Choose a reason for hiding this comment

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

DLGM. @NickCao have the right to merge this.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 20, 2025
@tomjnixon
Copy link
Contributor Author

Thanks for the review -- i've made the requested changes.

The github actions failures are ephemeral, it builds fine and is formatted correctly.

@qbisi
Copy link
Contributor

qbisi commented Apr 21, 2025

Thanks for the review -- i've made the requested changes.

The github actions failures are ephemeral, it builds fine and is formatted correctly.

Rebase on origin/master branch and force push again. If the ofborg success, wait for a week before the commiter merge this pr.

@NickCao NickCao merged commit 6aaa361 into NixOS:master Apr 28, 2025
29 checks passed
@tomjnixon
Copy link
Contributor Author

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants