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

horizon-eda: init at 1.1.1 #86694

Open
wants to merge 1 commit into
base: master
from
Open

horizon-eda: init at 1.1.1 #86694

wants to merge 1 commit into from

Conversation

@yrashk
Copy link
Contributor

yrashk commented May 4, 2020

Mostly based on #56497 by @Luz

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Member

emilazy left a comment

Needs a rebase, but builds and runs for me afterwards; thanks for working on this!

Please use 2 spaces for indentation rather than the current mix of hard tabs and 2-or-8 space indentation. I've left some more specific comments.

@@ -0,0 +1,50 @@
{ stdenv, fetchFromGitHub, pkgconfig, sqlite, libyamlcpp, libuuid, gnome3, epoxy, librsvg, zeromq, cppzmq, glm, libgit2, curl, boost, python3, opencascade, libzip, podofo, wrapGAppsHook, coreutils }:

This comment has been minimized.

Copy link
@emilazy

emilazy May 13, 2020

Member

Overly long line; should be split into multiple lines and preferably include lib directly rather than through stdenv; I personally prefer one package per line like this:

-{ stdenv, fetchFromGitHub, pkgconfig, sqlite, libyamlcpp, libuuid, gnome3, epoxy, librsvg, zeromq, cppzmq, glm, libgit2, curl, boost, python3, opencascade, libzip, podofo, wrapGAppsHook, coreutils }:
+{ stdenv
+, lib
+, fetchFromGitHub
+, pkgconfig
+, sqlite
+, libyamlcpp
+, libuuid
+, gnome3
+, epoxy
+, librsvg
+, zeromq
+, cppzmq
+, glm
+, libgit2
+, curl
+, boost
+, python3
+, opencascade
+, libzip
+, podofo
+, wrapGAppsHook
+, coreutils
+}:
version = "1.1.0";

src = fetchFromGitHub {
owner = "horizon-eda";
repo = "horizon";
rev = "v${version}";
sha256 = "1p8msmpxr5m2037yri3ml5h737n6cdgz2g817805z9kgilxdc56a";
};
Comment on lines 5 to 12

This comment has been minimized.

Copy link
@emilazy

emilazy May 13, 2020

Member

New upstream release:

-	version = "1.1.0";
+  version = "1.1.1";
 
-	src = fetchFromGitHub {
-		owner = "horizon-eda";
-		repo = "horizon";
-		rev = "v${version}";
-		sha256 = "1p8msmpxr5m2037yri3ml5h737n6cdgz2g817805z9kgilxdc56a";
-	};
+  src = fetchFromGitHub {
+    owner = "horizon-eda";
+    repo = "horizon";
+    rev = "v${version}";
+    hash = "sha256-gfCD//eiW7zmvVSeIVWos34lOTHqef0k96i4lJ1EzSg=";
+  };
zeromq
];

nativeBuildInputs = [ pkgconfig wrapGAppsHook coreutils ];

This comment has been minimized.

Copy link
@emilazy

emilazy May 13, 2020

Member

No need for coreutils here. It's implicitly present for every build, and you don't need to reference things in the build inputs to refer to them directly with a ${coreutils}/... path like in the installPhase; Nix tracks the dependencies automatically.

-	nativeBuildInputs = [ pkgconfig wrapGAppsHook coreutils ];
+  nativeBuildInputs = [ pkgconfig wrapGAppsHook ];

enableParallelBuilding = true;

meta = with stdenv.lib; {

This comment has been minimized.

Copy link
@emilazy

emilazy May 13, 2020

Member

If depending on lib directly:

-       meta = with stdenv.lib; {
+  meta = with lib; {
@yrashk yrashk force-pushed the yrashk:horizon-eda branch from 2944424 to 4c85254 May 13, 2020
@yrashk
Copy link
Contributor Author

yrashk commented May 13, 2020

@emilazy I believe I addressed your feedback in the updated commit.

@yrashk yrashk changed the title horizon-eda: init at 1.1.0 horizon-eda: init at 1.1.1 May 13, 2020
@emilazy emilazy mentioned this pull request May 13, 2020
1 of 10 tasks complete
Copy link
Member

emilazy left a comment

Thanks for addressing these! A couple more formatting nitpicks, but other than that this looks good to go to me.

The build failed on 64-bit ARM due to freeimage's vendored libpng; I tried to fix it but got lost in the weeds. It's already tracked in #77653, so I opened #86694 to mark freeimage as broken on AArch64 for now.

buildInputs = [
boost
cppzmq
curl
epoxy
glm
gnome3.gtkmm
libgit2
librsvg
libuuid
libyamlcpp
libzip
opencascade
podofo
python3
sqlite
zeromq
];
Comment on lines 35 to 52

This comment has been minimized.

Copy link
@emilazy

emilazy May 13, 2020

Member

Incorrect indentation and trailing whitespace at the end of lines:

-        buildInputs = [ 
-          boost
-          cppzmq 
-          curl
-          epoxy
-          glm
-          gnome3.gtkmm
-          libgit2
-          librsvg
-          libuuid 
-          libyamlcpp 
-          libzip
-          opencascade 
-          podofo
-          python3
-          sqlite
-          zeromq 
-        ];
+  buildInputs = [
+    boost
+    cppzmq
+    curl
+    epoxy
+    glm
+    gnome3.gtkmm
+    libgit2
+    librsvg
+    libuuid
+    libyamlcpp
+    libzip
+    opencascade
+    podofo
+    python3
+    sqlite
+    zeromq
+  ];
installPhase = ''
make install INSTALL=${coreutils}/bin/install DESTDIR=$out PREFIX=
'';
Comment on lines 58 to 60

This comment has been minimized.

Copy link
@emilazy

emilazy May 13, 2020

Member

Incorrect indentation:

   installPhase = ''
-                make install INSTALL=${coreutils}/bin/install DESTDIR=$out PREFIX=
+    make install INSTALL=${coreutils}/bin/install DESTDIR=$out PREFIX=
   '';
Mostly based on #56497 by @Luz
@yrashk yrashk force-pushed the yrashk:horizon-eda branch from 4c85254 to 50c7b77 May 13, 2020
@yrashk
Copy link
Contributor Author

yrashk commented May 13, 2020

@emilazy addressed your indentation remarks

@emilazy
Copy link
Member

emilazy commented May 13, 2020

Looks good, thanks!

@carrotIndustries

This comment has been minimized.

not needed since 1.1.0

@carrotIndustries

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.