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

samba 4.14.5 (new formula) #80171

Closed
wants to merge 1 commit into from
Closed

Conversation

AkihiroSuda
Copy link
Contributor

Samba (v3) was removed from Homebrew in 2016 due to technical issues (Homebrew/legacy-homebrew#17820, 5af93ae), however, Samba v4 seems actively supporting macOS recently, and worth including again in Homebrew.

Samba v4 is already included in MacPorts.

Further discussion in Homebrew/discussions#1736

The formula is based on Alexander Richardson (@arichardson)'s PR #32031 .

Example usage with qemu:

qemu-system-x86_64 \
  -accel hvf \
  -m 4096 \
  -cdrom ubuntu-21.04-desktop-amd64.iso \
  -net nic \
  -net user,smb=/tmp/foo

The shared folder can be mounted with mount.cifs //10.0.2.4/qemu /mnt in the guest.

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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>?

@BrewTestBot BrewTestBot added missing license Formula has a missing license which should be added new formula PR adds a new formula to Homebrew/homebrew-core labels Jun 28, 2021
@AkihiroSuda
Copy link
Contributor Author

cc @arichardson @fxcoudert

@BrewTestBot BrewTestBot removed the missing license Formula has a missing license which should be added label Jun 28, 2021
Formula/samba.rb Outdated
Comment on lines 45 to 50
# macOS has its own SMB daemon as /usr/sbin/smbd, so rename our smbd to samba-dot-org-smbd to avoid conflict.
# samba-dot-org-smbd is used by qemu.rb .
mv "#{sbin}/smbd", "#{sbin}/samba-dot-org-smbd"
Copy link
Member

Choose a reason for hiding this comment

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

This means the formulae should be keg-only instead of renaming IYAM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

QEMU requires the binary to be renamed:

# Sharing Samba directories in QEMU requires the samba.org smbd which is
# incompatible with the macOS-provided version. This will lead to
# silent runtime failures, so we set it to a Homebrew path in order to
# obtain sensible runtime errors. This will also be compatible with
# Samba installations from external taps.
args << "--smbd=#{HOMEBREW_PREFIX}/sbin/samba-dot-org-smbd"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should also be fine if it's called smbd but is inside a separate directory (i.e. not linked to /usr/local/sbin). I believe the reason I used a separate name was to make the error message more obvious.

Copy link
Member

Choose a reason for hiding this comment

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

QEMU requires the binary to be renamed

I think if we ship samba again we can just update the qemu formula

Copy link
Member

@carlocab carlocab Jun 28, 2021

Choose a reason for hiding this comment

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

I think if we ship samba again we can just update the qemu formula

Let's not do this. This breaks the install of anyone using a non-Homebrew Samba with Homebrew QEMU. Personally I think renaming one file to avoid a conflict is perfectly fine. We have a lot of formulae that do more than this to avoid becoming keg-only too. (e.g. the versioned gcc formulae)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can symlink it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

macOS version of smbd is completely different from Samba version of smbd except the binary name.

So I think renaming the binary without making it keg-only is more preferable than symlinking keg-only binary.

Formula/samba.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.

You have some errors from brew audit --new. Please don't mark items in the PR template as done when you did not do them.

Formula/samba.rb Outdated Show resolved Hide resolved
Formula/samba.rb Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda force-pushed the samba branch 2 times, most recently from 0e76cff to f47f74f Compare June 28, 2021 08:44
@arichardson
Copy link
Contributor

Is the groups_max patch no longer required? I don't see any changes to that function upstream. Without it samba fails if you are in more than 16 groups.

@arichardson
Copy link
Contributor

Thanks for working on this, will be great to have a binary package rather than having to build from source :)
For the Co-authored-by, you can use the following email: Co-authored-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>

@AkihiroSuda

This comment has been minimized.

@AkihiroSuda

This comment has been minimized.

@chenrui333 chenrui333 changed the title samba 4.14.5 samba 4.14.5 (new formula) Jun 28, 2021
@AkihiroSuda
Copy link
Contributor Author

Updated PR to use custom CFLAGS instead of applying patches, for avoiding Formulae should not require patches to build. Patches should be submitted and accepted upstream first. error.

Also added caveats to print the binary path ("/usr/local/sbin/samba-dot-org-smbd") and suggest running Samba as non-root.

@AkihiroSuda

This comment has been minimized.

@BrewTestBot BrewTestBot added the perl Perl use is a significant feature of the PR or issue label Jul 6, 2021
@AkihiroSuda AkihiroSuda force-pushed the samba branch 4 times, most recently from a44cfab to d1dfbf2 Compare July 6, 2021 12:57
Formula/samba.rb Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda force-pushed the samba branch 2 times, most recently from d09d4d2 to 83a08b0 Compare July 7, 2021 07:56
Formula/samba.rb Outdated Show resolved Hide resolved
Formula/samba.rb Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda force-pushed the samba branch 2 times, most recently from 4bd94d1 to 4ba89ac Compare July 7, 2021 10:08
Formula/samba.rb Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda marked this pull request as draft July 7, 2021 11:41
@AkihiroSuda AkihiroSuda force-pushed the samba branch 2 times, most recently from ae2ed6e to 6ec02e9 Compare July 8, 2021 04:51
Samba (v3) was removed from Homebrew in 2016 due to technical issues,
however, Samba v4 seems actively supporting macOS recently, and worth
including again in Homebrew.

Samba v4 is already included in MacPorts.

Further discussion in Homebrew/discussions 1736 .

The formula is based on Alexander Richardson (arichardson)'s PR 32031 .

Example usage with qemu:
```
qemu-system-x86_64 \
  -accel hvf \
  -m 4096 \
  -cdrom ubuntu-21.04-desktop-amd64.iso \
  -net nic \
  -net user,smb=/tmp/foo
```

The shared folder can be mounted with `mount.cifs //10.0.2.4/qemu /mnt`
in the guest.

Co-authored-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
@AkihiroSuda AkihiroSuda marked this pull request as ready for review July 8, 2021 05:31
end
# CFLAGS is for avoiding hitting https://bugzilla.samba.org/show_bug.cgi?id=14680 .
# Remove this CFLAGS when the patch ( https://attachments.samba.org/attachment.cgi?id=16579 ) gets merged.
ENV.append "CFLAGS", "-include #{buildpath}/lib/util/debug.h"
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a pretty heavy-handed way of getting around the no-patches requirement.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@github-actions github-actions bot added the outdated PR was locked due to age label Aug 17, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age perl Perl use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants