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

python3Packages.dm-tree: refactor to build from sources #152971

Merged
merged 2 commits into from Jan 1, 2022

Conversation

ndl
Copy link
Contributor

@ndl ndl commented Dec 31, 2021

Motivation for this change

Part of "extra JAX libraries / frameworks" implementation, see the discussion in #152754

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/)
  • 22.05 Release Notes (or backporting 21.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.

Comment on lines 28 to 31
-set(ABSEIL_VER 20210324.2)
-include(ExternalProject)
-ExternalProject_Add(abseil-cpp
- GIT_REPOSITORY https://github.com/abseil/abseil-cpp.git
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of vendored source is an anti-pattern. Would be nice if upstream supported find_package

Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker for this PR though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased to the most recent commit and changed the patch to hopefully be submittable to the upstream (= falling back to vendored deps if find_package fails), will try to open PR with them but keeping the patch here for now as I want to decouple upstream PR from this one.

pkgs/development/python-modules/dm-tree/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@samuela samuela left a comment

Choose a reason for hiding this comment

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

LGTM after comments are addressed

pkgs/development/python-modules/dm-tree/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/dm-tree/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@samuela samuela left a comment

Choose a reason for hiding this comment

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

LGTM I'll squash and merge tomorrow unless anyone objects.

@ofborg ofborg bot requested a review from samuela December 31, 2021 22:09
@samuela samuela merged commit 08322e6 into NixOS:master Jan 1, 2022
simeoncarstens added a commit to simeoncarstens/nixpkgs that referenced this pull request Jan 19, 2022
tensorflow-probability 0.8 was broken and marked as such (in NixOS#108977)
because of a dependency on dm-tree, which seems not to have been
available.
Since dm-tree is now available in nixpkgs (NixOS#152971),
tensorflow-probability is easy to fix and to upgrade.
simeoncarstens added a commit to simeoncarstens/nixpkgs that referenced this pull request Jan 26, 2022
tensorflow-probability 0.8 was broken and marked as such (in NixOS#108977)
because of a dependency on dm-tree, which seems not to have been
available.
Since dm-tree is now available in nixpkgs (NixOS#152971),
tensorflow-probability is easy to fix and to upgrade.
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.

None yet

3 participants