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

Jump to bottom

Gns3: fix build and crash #90631

Closed
wants to merge 3 commits into from
Closed

Gns3: fix build and crash #90631

wants to merge 3 commits into from

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Jun 16, 2020 •
edited

Motivation for this change

Builds broke in trunk-combined and it would crash on startup in my system

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.

@Atemu
gns3-gui: don't override jsonschema anymore
93deb26 
It needs 3.2.0 now
@Atemu
gns3-gui: add missing qt5Full runtime dependency
b368758 
qt5Full may not be installed on users' systems and the gns3-gui depends on it
explicitly
@Atemu
gns3-server: update version overrides
9698a56 
It needs jsonschema==3.2.0 and aiofiles==0.4.0
@Atemu Atemu requested a review from primeos June 16, 2020 17:53
primeos
primeos requested changes Jun 17, 2020
View reviewed changes
Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I had to push the changes manually, because:

  • 9698a56 is already in staging-next: 8c3e931
  • 93deb26: A modified version is now in staging-next: 06aec42
  • b368758: It's probably best to wait with this until staging-next is merged into master.

Note: I only selected request changes so that no one merges this PR on accident, but no actions are required.

@primeos primeos self-assigned this Jun 17, 2020
@Atemu
Copy link
Member Author

Atemu commented Jun 17, 2020

Thanks!

It might be a good idea to merge the qt5Full patch despite the build still being broken as it's a very trivial fix and could then be cherry-picked to 20.03 whose (working) builds are also affected by the crash.

@primeos
Copy link
Member

primeos commented Jun 18, 2020

It might be a good idea to merge the qt5Full patch despite the build still being broken as it's a very trivial fix and could then be cherry-picked to 20.03 whose (working) builds are also affected by the crash.

Yes, that would be nice. However, a problem is that GNS3 doesn't always crash (even when qt5Full isn't installed). I think there should be a better solution than adding qt5Full (approx. 1.5 GB), but we didn't find one so far. I'll try to have a closer look at this. Did you test/consider any alternatives?

@Atemu
Copy link
Member Author

Atemu commented Jun 18, 2020

The reason it doesn't crash sometimes might be due to the missing QT5 dependency being accessible system-wide already because of another package the user installed.

IMO having to download 1.5GiB (though I only had to download a few MiB) is a lot better than having the application crash on you, needing to google a Nixpkgs issue with a solution and then having to install qt5Full manually.

If we find the actual dependency later, we can reduce the closure size again.

@primeos
Copy link
Member

primeos commented Jun 24, 2020

The reason it doesn't crash sometimes might be due to the missing QT5 dependency being accessible system-wide already because of another package the user installed.

Yeah and in my case it probably only crashed when those dependencies where incompatible or something like that.

IMO having to download 1.5GiB (though I only had to download a few MiB) is a lot better than having the application crash on you, needing to google a Nixpkgs issue with a solution and then having to install qt5Full manually.

Agreed, I'm mostly concerned that the whole packaging is a bit broken atm (wrapQtAppsHook, etc. would be better; see https://nixos.org/nixpkgs/manual/#sec-language-qt).

If we find the actual dependency later, we can reduce the closure size again.

Yeah, that'd be best. But since I also lack time to investigate atm I pushed your commit for now: 0eaec4d (and backported in 6460602).
(I waited for the merge of staging as testing with qt5Full required too many rebuilds when I tried it and things where broken anyway.)

Closing this PR then as all changes are merged. Thanks!

@primeos primeos closed this Jun 24, 2020
@Atemu Atemu deleted the gns3-fixes branch July 30, 2020 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@primeos primeos primeos requested changes

Assignees

@primeos primeos

Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Atemu @primeos