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.

Sign up for GitHub

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

libvirt: add upstream patch to revert jansson #45409

Closed
wants to merge 1 commit into from
Closed

libvirt: add upstream patch to revert jansson #45409

wants to merge 1 commit into from

Conversation

VShell
Copy link
Contributor

@VShell VShell commented Aug 20, 2018 •
edited

Motivation for this change

https://bugzilla.redhat.com/show_bug.cgi?id=1614569

Fix has been merged into upstream master, but has not made it to a release yet. Managing QEMU virtual machines is completely broken without this fix.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

I've tested libvirtd and virt-manager using the patched libvirt, and the problem is resolved.

@VShell @alyssais
libvirt: add upstream patch to revert jansson
0354644 
https://bugzilla.redhat.com/show_bug.cgi?id=1614569

Fix has been merged into upstream master, but has not made it to a
release yet. Managing QEMU virtual machines is completely broken
without this fix.

Co-authored-by: Alyssa Ross <hi@alyssa.is>
@xeji
Copy link
Contributor

xeji commented Aug 21, 2018

@volth I agree. This patch actually contains multiple reverts from upstream master combined into a single large (>2500 line) patch file. I don't think we should do this.

I'd rather revert to 4.5.0 and wait for the next release. AFAIK there was nothing wrong with 4.5.0.

cc @LnL7 @pikajude

@xeji
Copy link
Contributor

xeji commented Aug 21, 2018

cc @fpletz

@LnL7
Copy link
Member

LnL7 commented Aug 21, 2018

Is there a problem with this? #45324

@xeji
Copy link
Contributor

xeji commented Aug 21, 2018

No, upstream has reverted their move from yajl to jansson because apparently there were some major issues. But they have not released a fixed version yet. So either we apply these many patches (I don't like committing a 2550 line patch file to our repo) or we undo our attempts to fix it and revert to 4.5.0.

@LnL7
Copy link
Member

LnL7 commented Aug 21, 2018

Hmm, yeah I think we should just revert back to the previous version then. Unless there's a specific not to do that?

@xeji
Copy link
Contributor

xeji commented Aug 21, 2018

Unless there's a specific not to do that?

Don't think so. According to https://libvirt.org/news.html 4.6.0 had no notable bugfixes, and 4.5.0 seemed to work fine.

@LnL7
Copy link
Member

LnL7 commented Aug 21, 2018

Let's go back to the previous version instead then.

@xeji
Copy link
Contributor

xeji commented Aug 21, 2018

working on a PR to revert right now.

@xeji xeji mentioned this pull request Aug 21, 2018
Merged
@fpletz
Copy link
Member

fpletz commented Aug 21, 2018

Thanks for your work on this. I also think we should revert for now.

@fpletz fpletz closed this Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@VShell @xeji @LnL7 @fpletz @GrahamcOfBorg