Conversation
github-actions
bot
added
the
6.topic: python
|
@samuela I took the liberty of switching |
ofborg
bot
added
10.rebuild-darwin: 11-100
|
Oh very nice! I myself packaged a few of these on my fork of nixpkgs (https://github.com/samuela/nixpkgs/tree/scratch) but never got around to contributing them upstream. I think these changes all look good to me broadly speaking. However, my preference is to keep PRs as small as possible. Would you mind sending PRs for each package independently? I know that is a bit more work on your part, but I promise to review and merge them as quickly as possible to keep the process moving along smoothly for you.
Yes, I think that's great! |
Sure, can do that - but note that some packages are inter-dependent (e.g. What do you think about grouping the dependent packages into PRs? That would mean the following PRs list:
There's still a dependency from (4) to (2) + (3) but at least that's one dependency and the rest can proceed independently. Of course, if you still prefer individual packages - we can do that as well, it will just take longer because we need to do many of them sequentially then. |
jonringer
left a comment
jonringer
left a comment
There was a problem hiding this comment.
for large package additions like this, sometimes it's easier if you chunk up the individual packges into smaller PRs. Then we can merge them as they become ready.
| -set(ABSEIL_VER 20210324.2) | ||
| -include(ExternalProject) | ||
| -ExternalProject_Add(abseil-cpp | ||
| - GIT_REPOSITORY https://github.com/abseil/abseil-cpp.git | ||
| - GIT_TAG ${ABSEIL_VER} | ||
| - PREFIX ${CMAKE_SOURCE_DIR}/abseil-cpp | ||
| - CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_SOURCE_DIR}/abseil-cpp | ||
| - -DCMAKE_CXX_STANDARD=${CMAKE_CXX_STANDARD} | ||
| - -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER} | ||
| - -DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS} | ||
| - -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} | ||
| - -DCMAKE_POSITION_INDEPENDENT_CODE=${CMAKE_POSITION_INDEPENDENT_CODE} | ||
| - -DLIBRARY_OUTPUT_PATH=${CMAKE_SOURCE_DIR}/abseil-cpp/lib | ||
| -) | ||
| -ExternalProject_Get_Property(abseil-cpp install_dir) | ||
| -set(abseil_install_dir ${install_dir}) | ||
| -include_directories (${abseil_install_dir}/include) | ||
| - | ||
| +find_package(absl REQUIRED) | ||
|
|
There was a problem hiding this comment.
this doesn't have to be a blocker for this PR, but it might be nice to upstream the package with a find_package ... fallback to external type of workflow.
There was a problem hiding this comment.
Ack, will see if I can upstream it separately, will keep the patch meanwhile.
| src = fetchFromGitHub { | ||
| owner = "cgarciae"; | ||
| repo = "${pname}"; | ||
| rev = "${version}"; |
There was a problem hiding this comment.
| rev = "${version}"; | |
| rev = version; |
I'd still prefer individual packages, but on these sorts of questions I defer to maintainers that have more experience in nixpkgs than I, eg @jonringer. I know it's annoying in the short term but it really helps to keep the git blame clean and I promise to give you a fast turnaround time on any issues I'm tagged in! |
I've also seen large PRs have many small nitpicks which block the PR. It can be very draining as well to have "0 progress" over a long period |
|
Per discussion above, closing in favor of individual PRs. I've migrated all the changes to the relevant commits, so I believe no review comments were lost. |
3 participants
Motivation for this change
This PR packages some of the commonly-used JAX frameworks / libraries.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes