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: Add 9p support for darwin #96655

Closed
wants to merge 2 commits into from
Closed

Conversation

baude
Copy link
Contributor

@baude baude commented Mar 10, 2022

Qemu already has support for 9p in Linux. Adding this for MacOS users
will bring them into parity for 9p support.

This replaces Ashley's #95318

Signed-off-by: Brent Baude [email protected]

  • [X ] Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • [X ] Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@baude
Copy link
Contributor Author

baude commented Mar 10, 2022

@SMillerDev This replaces Ashley's PR. I am less experience to brew so appreciate the review.

@SMillerDev
Copy link
Member

Could you run brew style --fix homebrew/core/qemu?

@baude
Copy link
Contributor Author

baude commented Mar 10, 2022

I ran it but nothing in git appears to have changed. Is that expected?

@SMillerDev
Copy link
Member

Are you working in a new clone of homebrew core?

@baude
Copy link
Contributor Author

baude commented Mar 10, 2022

Are you working in a new clone of homebrew core?

@SMillerDev Yessir.

@SMillerDev
Copy link
Member

Yeah, that's the issue then.

You can also point brew style --fix to the file I believe.

The general recommendation is to work in your live brew install.
https://docs.brew.sh/How-To-Open-a-Homebrew-Pull-Request#formulae-related-pull-request

@baude
Copy link
Contributor Author

baude commented Mar 10, 2022

@SMillerDev So as a noob here, should I be doing what the docs recommend which is to get the patches to apply and then do a git diff on the source code and capture that as foo.diff? And then foo.diff would just be included versus downloaded?

@SMillerDev
Copy link
Member

No, the downloaded option is definitely easier. Just change the file extension you're downloading.

@carlocab carlocab changed the title Add 9p support for darwin qemu: Add 9p support for darwin Mar 10, 2022
@baude
Copy link
Contributor Author

baude commented Mar 10, 2022

We good?

@cho-m
Copy link
Member

cho-m commented Mar 11, 2022

Linux build failure:

../meson.build:1386:0: ERROR: Feature virtfs cannot be enabled: virtio-9p (virtfs) on Linux requires libcap-ng-devel and libattr-devel

Needs Linux-only dependencies within the on_linux do block. Maybe:

   on_linux do
+    depends_on "attr"
     depends_on "gcc"
     depends_on "gtk+3"
+    depends_on "libcap-ng"
   end

Formula/qemu.rb Outdated Show resolved Hide resolved
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

We probably need to increment this formula's revision.

When making changes to this PR, please amend your existing commit instead of adding new ones. When you do so, change your commit message to

qemu: add 9p support for Darwin

(or similar) so that it conforms to the commit style guide.

Formula/qemu.rb Show resolved Hide resolved
Formula/qemu.rb Show resolved Hide resolved
Formula/qemu.rb Show resolved Hide resolved
Formula/qemu.rb Outdated Show resolved Hide resolved
@baude
Copy link
Contributor Author

baude commented Mar 11, 2022

We probably need to increment this formula's revision.

I fixed up the other issues. Is incrementing something I do? If so, can you please elaborate?

@willcohen
Copy link
Contributor

willcohen commented Mar 11, 2022

It'd just be adding revision 1 under the head line near the top, I think!

https://rubydoc.brew.sh/Formula#revision%3D-class_method

Formula/qemu.rb Outdated Show resolved Hide resolved
@baude baude force-pushed the qemu9pdarwin branch 2 times, most recently from 7879785 to f1c3fd5 Compare March 11, 2022 16:48
@willcohen
Copy link
Contributor

Sorry for getting that wrong — GH is telling me that revision goes before head…

qemu: add 9p support for Darwin

Qemu already has support for 9p in Linux.  Adding this for MacOS users
will bring them into parity for 9p support.

This replaces Ashley's Homebrew#95318

Signed-off-by: Brent Baude <[email protected]>
@baude
Copy link
Contributor Author

baude commented Mar 11, 2022

What happened that two of the tasks were cancelled?

@cho-m cho-m added long build Set a long timeout for formula testing CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. labels Mar 11, 2022
@trevor-viljoen
Copy link

LGTM

@SMillerDev
Copy link
Member

Can you add the patches from #96743 too?

Replaces Homebrew#96743 courtesy of AkihiroSuda

Signed-off-by: Brent Baude <[email protected]>
@baude
Copy link
Contributor Author

baude commented Mar 13, 2022

I've added #96743 as asked ... I think the a maintainer needs to set the long ci timeout label?

@cho-m cho-m added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Mar 13, 2022
@carlocab carlocab removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Mar 13, 2022
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks!

@carlocab carlocab added the automerge-skip `brew pr-automerge` will skip this pull request label Mar 13, 2022
@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@saschasc
Copy link
Contributor

Great to have 9p support soon!

I noticed that if I compile QEMU myself and run brew install spice-protocol beforehand, the clipboard sharing also works afterwards when compiling it from source. Would it be possible to add this to QEMU as well? Where would be the best place to request this? Who could help?

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 13, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request long build Set a long timeout for formula testing outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants