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

ipbus-uhal: init at 2.8.16 #357608

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joealsubash
Copy link

IPBus uhal and firmware provide a mechanism to read/write into FPGA firmware in a reliable fashion. It is used in many current particle-physics experiments at CERN like ATLAS and CMS.
shell-env.sh allows development using the uhal API and provides python bindings for reading and writing into firmware.

https://ipbus.web.cern.ch/

Things done

  • Built on platform(s)
    • [ x] 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
  • 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.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Nov 20, 2024
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Welcome to nixpkgs!
Please modify your pr title and commit message to comply with the repository requirements
Please add this package in pkgs/top-level/all-packages.nix

pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from b4b7c15 to fbed328 Compare November 21, 2024 12:34
@joealsubash joealsubash changed the title IPBus uhal integration into nixpkgs ipbus-uhal: init at 2.8.16 : IPBus uhal integration into nixpkgs Nov 21, 2024
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 21, 2024
@joealsubash
Copy link
Author

Hi,
Thanks for the review.
I have tried to incorporate your suggestions. Could you point me in the right direction as to how to fix the Vet nixpkgs error?

@joealsubash joealsubash changed the title ipbus-uhal: init at 2.8.16 : IPBus uhal integration into nixpkgs ipbus-uhal: init at 2.8.16 Nov 25, 2024
@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from 8812c99 to e11cbca Compare November 25, 2024 13:43
@joealsubash
Copy link
Author

Hi, I have squashed the commits and added it to pkgs/top-level/all-packages.nix, please review and let me know anymore changes that are required. Kindly merge otherwise. Thanks.

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
@@ -0,0 +1,47 @@
#! @bash@/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really quite unsure of what the purpose of this script is.

Copy link
Author

Choose a reason for hiding this comment

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

Its probably because I don't understand nix very well.
But the idea is, package user should be able to invoke uhal from python and use it to communicate with FPGA hardware connected via. the Ethernet port. That is what adding to PYTHONPATH part does. Another use case is for software developers to be able to access the uhal API to write their custom C/C++ applications.
These are the main ideas this script is trying to provide when the user wants a nix shell with uhal.
If there is a better way to do it, I'd be happy to change. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

In Nix shells, if you want to have a Python install with certain modules configured, you'd use the withPackages function on python3: python3.withPackages (ps: with ps; [ iphus-uhal ]) or something similar.

As for installing other tools, you should add them to propagatedBuildInputs.

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 26, 2024
Copy link
Contributor

@pluiedev pluiedev left a comment

Choose a reason for hiding this comment

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

All other unresolved comments still remain an issue

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
pkgs/development/embedded/fpga/ipbus-uhal/uhal.nix Outdated Show resolved Hide resolved
@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from 936dcf8 to 2605ba1 Compare November 27, 2024 15:11
@github-actions github-actions bot removed the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 27, 2024
@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from 1dde288 to cdd4eb1 Compare November 27, 2024 15:28
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 27, 2024
@joealsubash
Copy link
Author

Hi, does this look ok to merge now? Thanks.

@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 labels Nov 28, 2024
@pluiedev
Copy link
Contributor

I don't think my concerns regarding the install prefix & the superfluous bash script have been resolved yet

@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from cdd4eb1 to f6e2751 Compare December 2, 2024 16:44
@joealsubash
Copy link
Author

I got the following error and cannot build.

uhal> Traceback (most recent call last):
uhal>   File "/build/source/uhal/python/rpm/RPMBUILD/cactuscore-uhal-python312.py", line 3, in <module>
uhal>     from distutils.core import setup
uhal>   File "/nix/store/w3kv27g00v7g7k2kw5yf51wlbjsy273n-python3.12-distutils-75.1.0/lib/python3.12/site-packages/distutils/core.py", line 17, in <module>
uhal>     from .dist import Distribution
uhal>   File "/nix/store/w3kv27g00v7g7k2kw5yf51wlbjsy273n-python3.12-distutils-75.1.0/lib/python3.12/site-packages/distutils/dist.py", line 17, in <module>
uhal>     from packaging.utils import canonicalize_name, canonicalize_version
uhal> ModuleNotFoundError: No module named 'packaging'
uhal> make[2]: *** [/build/source/uhal/config/mfPythonRPMRules.mk:97: install] Error 1
uhal> make[2]: Leaving directory '/build/source/uhal/python'
uhal> make[1]: *** [Makefile:49: python/.virtual.Makefile] Error 2
uhal> make[1]: Leaving directory '/build/source/uhal'
uhal> make: *** [Makefile:58: uhal/.virtual.Makefile] Error 2

I am unable to reproduce your error locally(builds fine on my machine), not sure why. How can I investigate this further?
'enableParallelBuilding = true' sped things up a lot, thanks.

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 6, 2024
@FliegendeWurst
Copy link
Member

The package builds for me. Some suggestions:

diff --git a/pkgs/by-name/ip/ipbus-uhal/package.nix b/pkgs/by-name/ip/ipbus-uhal/package.nix
index 8fc83623ebf6..57e920610b75 100644
--- a/pkgs/by-name/ip/ipbus-uhal/package.nix
+++ b/pkgs/by-name/ip/ipbus-uhal/package.nix
@@ -34,23 +34,22 @@ stdenv.mkDerivation (finalAttrs: {
     python3.pkgs.distutils
     python3.pkgs.pybind11
   ];
-  configurePhase = ''
-    patchShebangs  uhal/config/install.sh
-    patchShebangs uhal/tests/setup.sh
-    patchShebangs scripts/doxygen/api_uhal.sh
-    patchShebangs config/progress.sh
-    patchShebangs config/Makefile.macros
+  postPatch = ''
+    patchShebangs --build uhal/config/install.sh
+    patchShebangs --build uhal/tests/setup.sh
+    patchShebangs --build scripts/doxygen/api_uhal.sh
+    patchShebangs --build config/progress.sh
+    patchShebangs --build config/Makefile.macros
   '';
 
   enableParallelBuilding = true;
 
-  makeFlags = [ "Set=uhal" ];
-
-  installPhase = ''
-    mkdir -p $out/bin
-    mkdir -p $out/include
-    make Set=uhal install prefix=$out/bin includedir=$out/include
-  '';
+  makeFlags = [
+    "Set=uhal"
+    "CXX=${stdenv.cc.targetPrefix}c++"
+    "prefix=$out/bin"
+    "includedir=$out/include"
+  ];
 
   meta = {
     description = "Software which pairs with ipbus-firmware";

@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from 31a4b4a to b4b22cc Compare January 21, 2025 14:34
@joealsubash
Copy link
Author

Hi,
I have tested this on a few machines and seems to be building and working ok now. Could this be merged now?
Thanks

pkgs/by-name/ip/ipbus-uhal/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ip/ipbus-uhal/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ip/ipbus-uhal/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ip/ipbus-uhal/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ip/ipbus-uhal/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ip/ipbus-uhal/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ip/ipbus-uhal/package.nix Outdated Show resolved Hide resolved
@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch 2 times, most recently from 66f1c25 to 2f4639c Compare January 21, 2025 16:04
@github-actions github-actions bot added 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` and removed 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Jan 21, 2025
@joealsubash
Copy link
Author

@FliegendeWurst how does this look now? I have incorporated your changes, but had to revert some because it broke the installation. Thanks for the review and suggestions.

@FliegendeWurst
Copy link
Member

boost186 and the tag argument still apply. You might need to rebase on current master for it to work.

@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch 3 times, most recently from 26691dd to 2dc271b Compare January 22, 2025 15:53
@joealsubash
Copy link
Author

boost186 and the tag argument still apply. You might need to rebase on current master for it to work.

OK, those changes have been made now and has been rebased.

@FliegendeWurst
Copy link
Member

Thank you. Please also reorder the commits so the maintainers modification comes first, and clean up the other commit message a bit 😉

@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from 2dc271b to 7a44831 Compare January 23, 2025 09:46
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jan 23, 2025
Co-authored-by: aucub <[email protected]>

ipbus-uhal: init at 2.8.16 : removed old git source; not using "with lib" anymore; empty maintainers added

Co-authored-by: Leah Amelia Chen <[email protected]>

Co-authored-by: Leah Amelia Chen <[email protected]>

ipbus-uhal: init at 2.8.16 - remove pkgs.distutils and enableParallelBuilding

ipbus-uhal: init at 2.8.16 - re-add python3.pkgs.distutil after local build failure, fixed after adding

adding suggestions by FliegendeWurst

undoing some changes to allow installation; installation did'nt work with previous commit

fixing merge conflict

Co-authored-by: Arne Keller <[email protected]>

fix multiple strings

pre and postInstall hooks added

removing python stuff.

remove python stuff

using new shorthand

patching in pacakge.nix instead

changes to fix error when testing

removing unecessary patch

usign new tag format for fetchFromGitHub

sticking to boost1.86; ipbus-uhal ioservice dependence

Fixing Formatting using nixpkgs
@joealsubash joealsubash force-pushed the ipbus-uhal-in-nixpkgs branch from 7a44831 to 1d049a4 Compare January 23, 2025 11:23
@github-actions github-actions bot removed the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jan 23, 2025
@joealsubash
Copy link
Author

Thank you. Please also reorder the commits so the maintainers modification comes first, and clean up the other commit message a bit 😉

Ok done now. Made a mistake when rebasing but this has been fixed in the newest push.

@FliegendeWurst
Copy link
Member

Alright it looks okay now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 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