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

fritzing: 0.9.3b -> 0.9.4 #98381

Merged
merged 1 commit into from Oct 11, 2020
Merged

fritzing: 0.9.3b -> 0.9.4 #98381

merged 1 commit into from Oct 11, 2020

Conversation

avdv
Copy link
Member

@avdv avdv commented Sep 21, 2020

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
  • 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.

@ajs124
Copy link
Member

ajs124 commented Sep 21, 2020

#98277 also attempted this, but is marked as a draft.

@risicle
Copy link
Contributor

risicle commented Sep 21, 2020

Builds & runs for me, non-nixos linux x86_64.

Copy link
Member Author

@avdv avdv left a comment

Choose a reason for hiding this comment

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

#98277 also attempted this, but is marked as a draft.

@Sohalt I started to look into this on Friday, and didn't look back if there was something for fritzing... :-)

Maybe you can test this PR too. 👍

repo = "fritzing-parts";
rev = version;
sha256 = "1d2v8k7p176j0lczx4vx9n9gbg3vw09n2c4b6w0wj5wqmifywhc1";
parts = fetchgit {
Copy link
Member Author

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...

Copy link
Contributor

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.

@Sohalt
Copy link
Contributor

Sohalt commented Sep 22, 2020

#98277 also attempted this, but is marked as a draft.

@Sohalt I started to look into this on Friday, and didn't look back if there was something for fritzing... :-)

Maybe you can test this PR too. +1

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.
I don't know how often the parts library will get updated and how fast that update would land in nixpkgs, but seeing how it has been broken for a while I think it might take its time. The patch would somewhat help with that (but it would also conflict with your current solution of hardcoding the SHA).

@avdv
Copy link
Member Author

avdv commented Sep 22, 2020

The AUR package adds a patch to allow using a different fritzing-parts repository than the one that they ship.
I don't know how often the parts library will get updated and how fast that update would land in nixpkgs, but seeing how it has been broken for a while I think it might take its time. The patch would somewhat help with that (but it would also conflict with your current solution of hardcoding the SHA).

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...

Copy link
Member

@oxzi oxzi left a 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 ];

pkgs/applications/science/electronics/fritzing/default.nix Outdated Show resolved Hide resolved
mkDerivation rec {
pname = "fritzing";
version = "0.9.3b";
version = "0.9.4";
Copy link
Member

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.

Suggested change
version = "0.9.4";
version = "0.9.4-${fritzingTag}";

Copy link
Member Author

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...?!

Copy link
Member

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.

Copy link
Member Author

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...

Copy link
Member

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 (:

oxzi added a commit to hackspace-marburg/co2ampel that referenced this pull request Sep 24, 2020
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
@avdv
Copy link
Member Author

avdv commented Oct 5, 2020

I have squashed the changes together. Should be ready to merge now.

Caveat: I did only fix the build on Linux, since I cannot test on macOS. Should we use broken = lib.isDarwin here?

I have just noticed that this package has platforms = stdenv.lib.platforms.linux, so this should not be an issue right now.

Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

Tested successfully, LGTM.

Copy link
Contributor

@jonringer jonringer left a 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)

@avdv
Copy link
Member Author

avdv commented Oct 11, 2020

this looks to be correctly wrapped, however, I'm unable to open some qt plugins, so it fails to open:

@jonringer could you run it while setting QT_DEBUG_PLUGINS=1 to shed some light why this happens on your system?

Copy link
Contributor

@jonringer jonringer left a 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

@jonringer jonringer merged commit 0f6b8b7 into NixOS:master Oct 11, 2020
@avdv avdv deleted the fritzing-0.9.4 branch October 12, 2020 06:53
vcunat pushed a commit that referenced this pull request Oct 25, 2020
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.
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

6 participants