-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
fritzing: 0.9.3b -> 0.9.4 #98381
fritzing: 0.9.3b -> 0.9.4 #98381
Conversation
#98277 also attempted this, but is marked as a draft. |
Builds & runs for me, non-nixos linux x86_64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repo = "fritzing-parts"; | ||
rev = version; | ||
sha256 = "1d2v8k7p176j0lczx4vx9n9gbg3vw09n2c4b6w0wj5wqmifywhc1"; | ||
parts = fetchgit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to fetchgit
because fritzing tries to access the git repository and extract the Sha from it. So, I thought it would need the .git folder at runtime, but then I chose to hardcore the Sha into the program as a string literal.
This means it could be switched back to fetchFromGithub
again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we usually just patch out attempts at that kind of thing.
Tested and works for me 👍 Thank you! The AUR package adds a patch to allow using a different fritzing-parts repository than the one that they ship. |
That could be an improvement, indeed. But then, the latest fritzing-parts commit is from 19 Sep 2019. Almost seems like a wasted feature, being able to check and update the parts if there are no updates for months... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this Pull Request. I was just working on the same and thought about checking for existing PRs in the middle of it.
You do not need to use an old libgit2
version. There is already a patch upstream which we can backport. I have successfully tested your suggested PR with the following diff.
--- a/pkgs/applications/science/electronics/fritzing/default.nix
+++ b/pkgs/applications/science/electronics/fritzing/default.nix
@@ -1,5 +1,5 @@
-{ mkDerivation, stdenv, fetchgit, fetchFromGitHub, qmake, pkgconfig
-, qtbase, qtsvg, qttools, qtserialport, boost, libgit2_0_27
+{ mkDerivation, stdenv, fetchgit, fetchpatch, fetchFromGitHub, qmake, pkgconfig
+, qtbase, qtsvg, qttools, qtserialport, boost, libgit2
}:
let
@@ -30,7 +30,13 @@ mkDerivation rec {
sha256 = "0spka33a5qq34aq79j01arw1aly4vh0hzv7mahryhdlcdk22qqvc";
};
- buildInputs = [ qtbase qtsvg qtserialport boost libgit2_0_27 ];
+ patches = [(fetchpatch {
+ name = "fix-libgit2-version.patch";
+ url = "https://github.com/fritzing/fritzing-app/commit/472951243d70eeb40a53b1f7e16e6eab0588d079.patch";
+ sha256 = "0v1zi609cjnqac80xgnk23n54z08g1lia37hbzfl8jcq9sn9adak";
+ })];
+
+ buildInputs = [ qtbase qtsvg qtserialport boost libgit2 ];
nativeBuildInputs = [ qmake pkgconfig qttools ];
mkDerivation rec { | ||
pname = "fritzing"; | ||
version = "0.9.3b"; | ||
version = "0.9.4"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the fritzingTag
should also be added to the version
, because there multiple 0.9.4 releases, e.g., CD-415 and CD-498.
version = "0.9.4"; | |
version = "0.9.4-${fritzingTag}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not want to add this, as it looks ugly to me and is just a (mostly) random number. If this ever happens again and make multiple releases for a version, we might simply append a rev number at the end.
Otherwise, we could just append the build number, without the CD
prefix...?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally feel you and also consider it to be unaesthetic. However, the project does not use semantic versioning and "0.9.4" is ambiguous. Just adding the build number is also fine for me. I just want to be able to distinguish versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I changed the version to include the build number. I can squash the commits if wanted, or someone squashes when merging...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashing those into one commit sounds good, I think. Otherwise this PR looks good to me, thanks again (:
Added an overlay based on NixOS/nixpkgs#98381
Notes: * fritzing still needs an older version of libgit2 * releases no longer directly correspond to tags in the git repository, they are using build numbers instead * the fritzing-parts repository is no longer versioned at all, the master branch contains the latest stable release * a `parts.db` file needs to be generated from the fritzing-parts files during the build
I have squashed the changes together. Should be ready to merge now.
I have just noticed that this package has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested successfully, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks to be correctly wrapped, however, I'm unable to open some qt plugins, so it fails to open:
[nix-shell:~/.cache/nixpkgs-review/pr-98381-1]$ ./results/fritzing/bin/Fritzing
qt.qpa.xcb: could not connect to display
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.
Available platform plugins are: wayland-org.kde.kwin.qpa, wayland-egl, wayland, wayland-xcomposite-egl, wayland-xcomposite-glx, eglfs, linuxfb, minimal, minimalegl, offscreen, vnc, xcb.
Aborted (core dumped)
@jonringer could you run it while setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now able to open it fine. Not sure what changed, but the expression seems to follow nixpkgs Qt norms.
diff LGTM
gui launches fine
Notes: * fritzing still needs an older version of libgit2 * releases no longer directly correspond to tags in the git repository, they are using build numbers instead * the fritzing-parts repository is no longer versioned at all, the master branch contains the latest stable release * a `parts.db` file needs to be generated from the fritzing-parts files during the build (cherry picked from commit 0f6b8b7) Otherwise it wouldn't build. Master PR: #98381.
Motivation for this change
ZHF: #97479
Fritzing does not build currently, because it is not compatible with libgit2 1.x (they are using libgit 0.28.1 -- see https://github.com/fritzing/fritzing-app/blob/a799673d7f79a619d8c94a87d07cf0cebde1b1d9/.travis.yml#L87)
Also, a new version has been released on Dec, 1st 2019. See https://github.com/fritzing/fritzing-app/releases/tag/CD-498 (I could also separate the patches, if we only want to backport the fix of 0.9.3b to the release-20.09 branch).
Since their build scripts are a bit unorthodox, and I did not want to replicate all the different steps in the nixpkgs build again, I chose to patch the
tools/linux_release_script/release.sh
.Caveat: I did only fix the build on Linux, since I cannot test on macOS. Should we use
broken = lib.isDarwin
here?Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)