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

qemu: use shared libslirp instead of vendored #101608

Closed
wants to merge 3 commits into from
Closed

qemu: use shared libslirp instead of vendored #101608

wants to merge 3 commits into from

Conversation

@zowoq
Copy link
Contributor

@zowoq zowoq commented Oct 24, 2020

Motivation for this change

So we don't need to patch the vendored libslirp.

#76065
#79050

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.

cc @risicle

Copy link
Member

@andir andir left a comment

Can you add the reasoning you hinted at in the PR description also to the commit that adds the feature to QEMU? It would be nice to be able to judge why this was done just from the commit logs without yet another indirection.

@zowoq zowoq requested a review from andir Oct 24, 2020
@zowoq zowoq changed the title qemu: use shared libslirp qemu: use shared libslirp instead of vendored Oct 24, 2020
@mweinelt
Copy link
Member

@mweinelt mweinelt commented Oct 24, 2020

Can you add the reasoning you hinted at in the PR description also to the commit that adds the feature to QEMU? It would be nice to be able to judge why this was done just from the commit logs without yet another indirection.

The reason would likely be

So we don't need to patch the vendored libslirp.

Not sure why you left that out.

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Oct 24, 2020

I think use shared libslirp instead of vendored is accurate and sufficient as a commit message.

@andir
Copy link
Member

@andir andir commented Oct 25, 2020

I think use shared libslirp instead of vendored is accurate and sufficient as a commit message.

But it doesn't contain the why we did it. Stating what has been done can ofte be extracted from the changes but not the reason for that change.

Let me propose the following:

qemu: use shared libslirp


In the past we had to patch the bundled libslurp multiple times (e.g. #76065 &
#79050) while we have & had a proper package for libslurp. With this change we
can apply patches for libslurp once in a straight forward fashion and those
will also apply to QEMU.

@zowoq zowoq closed this Oct 25, 2020
@zowoq zowoq deleted the zowoq:slirp branch Oct 25, 2020
@grahamc
Copy link
Member

@grahamc grahamc commented Oct 25, 2020

I'm a bit disappointed to see you close the PR and delete the branch over what looks like a really good commit message suggestion...

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Oct 25, 2020

And I'm disappointed that a commit message that does explain the change is being bikeshedded.

Heaps of stuff gets merged with commit messages vastly worse than this.

If what I had isn't good enough I've got other things to do rather than argue.

@grahamc
Copy link
Member

@grahamc grahamc commented Oct 25, 2020

Okay, thanks.

@andir
Copy link
Member

@andir andir commented Oct 25, 2020

Just as reference: I am not trying to pick on you but I am trying to make sure everything I handle follows a good style. We even have that documented in the CONTRIBUTING.md

@risicle
Copy link
Contributor

@risicle risicle commented Oct 25, 2020

WFM macos 10.14, LGT ... oh.

@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Oct 25, 2020

I was being serious when I said "I've got other things to do rather than argue" but I see that this is now being discussed on discourse so I will respond. I'm not going to open an account on discourse.

I am not trying to pick on you but I am trying to make sure everything I handle follows a good style.

If you want people to follow a style, find a way for it to be enforced consistently tree-wide. Otherwise it isn't anything except unsustainable pedantry.

We even have that documented in the CONTRIBUTING.md

As we don't even enforce format the commit messages in the following way from CONTRIBUTING I'm actually kind of annoyed by writing good commit messages being used as some sort of justification here.

During a review that I did today a contributor decided to close their PR

s/contributor/committer/

@andir
Copy link
Member

@andir andir commented Oct 25, 2020

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Nov 27, 2020

Can we pick this up, please? If we had merged this we'd not be affected by CVE-2020-29129.

My bad, libslirp is affected, not QEMU itself. But I still believe this is a good idea.

@mweinelt mweinelt mentioned this pull request Nov 27, 2020
3 of 10 tasks
@zowoq
Copy link
Contributor Author

@zowoq zowoq commented Nov 27, 2020

I'll reopen if the request to change the commit message is withdrawn.

@andir
Copy link
Member

@andir andir commented Nov 28, 2020

I'll not withdraw that request. It should clearly state the motivation for this change.

@risicle
Copy link
Contributor

@risicle risicle commented Nov 28, 2020

I'm losing the will to live here.

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

5 participants