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

samba4: add package samba-4.8 #5563

Merged
merged 1 commit into from
Jul 30, 2018
Merged

samba4: add package samba-4.8 #5563

merged 1 commit into from
Jul 30, 2018

Conversation

Andy2244
Copy link
Contributor

@Andy2244 Andy2244 commented Feb 2, 2018

Maintainer: me
Compile tested: arm/mips (master)
Run tested: arm/mvebu (master)

Description:
This is my samba 4.8 package, it includes the server with vfs plugin support and tools, with extended config options (fileserver, netbios, ad-dc, winbind, avahi). It also includes a WSD Daemon to replace netbios, so Win10 clients can see samba shares via explorer.
The corresponding luci-ui package openwrt/luci#1985 was already merged.

Signed-off-by: Andy Walsh andy.walsh44+github@gmail.com

@Andy2244
Copy link
Contributor Author

Andy2244 commented Feb 3, 2018

Just confirmed on a fresh compile test, target x86 fails because e2fsprogs is build in-between krb5 and samba4.
@nwf send me patches for e2fsprogs/krb5 so libcom_err, libss are separate packages and can be reused by krb5, trying to test those now.

@poranje
Copy link
Contributor

poranje commented Feb 4, 2018 via email

@Andy2244
Copy link
Contributor Author

Andy2244 commented Feb 4, 2018

@poranje with #714 and #5562 krb5 accepts the system com_err/ss and in return samba4 accepts krb5 libs. As far as i understand com_err can be system-wide shared, but packages generate some static code with a compatible compile_et. At least thats what krb5 seems to-do, it tests/probes the system com_err lib and also the compile_et script.

@Andy2244
Copy link
Contributor Author

Updated to stable 4.8 branch and now passes travis checks, should still be merged alongside #5562.

@lucize
Copy link
Contributor

lucize commented Mar 30, 2018

also tested and working on mvebu due to samba3.6 not compiling anymore on my system (maybe gcc7.3 problems can't say)

@hnyman can we have this series in, samba 3.6 is long EOL ?

@Andy2244 everything in /usr/lib is needed(based on config options) ? did you tried file by file if they are all needed ?
Regards

@Andy2244
Copy link
Contributor Author

Andy2244 commented Mar 30, 2018

@lucize I cant judge if its "needed", but the final bins are linked against all those libs so they are needed to run the selected targets/features. I use everything that could be done with the default waf logic and without heavily patching the source to minimize size. So i only select the targets that are required for each feature and made the features select-able, so the minimal size is a server only (no netbios/vfs/ad-dc) package at around 4.2-4.6 MB size.
The main reason samba4 is so much bigger, is they removed the ability to use the old none waf build system to only compile the file server code, which is what most embedded samba 3.6 builds are based on.

If anyone finds a trick to minimize it more, i'm happy to add the patches as long as they are reasonable to maintain.

@lucize
Copy link
Contributor

lucize commented Mar 30, 2018

ok, I'll test it one by one and come back with an answer, maybe with some ifdef based on build selection some libs can be excluded

Regards

@Andy2244
Copy link
Contributor Author

Andy2244 commented Mar 30, 2018

@lucize You can try, but keep in mind that waf is already supposedly doing this. So if you tell waf to only build "smbstatus" than it will resolve all needed libs and only build/include those needed for "smbstatus". You can check the wafscripts for the bins and notice how each bin has specific dependency's defined.

We tried the static multicall bin trick a while ago, which ensures only needed functions are linked and the binary was around the same size as the shared waf package.

@diizzyy
Copy link
Contributor

diizzyy commented Apr 14, 2018

Is there a reason why we're not using packages for (lib)talloc and tdb?

@Andy2244
Copy link
Contributor Author

Andy2244 commented Apr 14, 2018

@diizzyy kinda, the existing lib packages are not using proper cross-compile answer files per architecture or qemu. So i doubt anyone actually tested the libs on all openwrt architectures. Thats why i use qemu in this package, since i did not want to guess cross-answers or update the files every few versions for all possible architectures.


config SAMBA4_SERVER
bool "Server (fileserver)"
select BUSYBOX_CONFIG_RENICE
Copy link
Contributor

@diizzyy diizzyy Apr 16, 2018

Choose a reason for hiding this comment

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

This is a bad idea, don't modify other packages "indirectly". Submit that it should be enabled by default upstream instead if it's really needed however my guess is that it's not the end of the world if renice isn't available.

Edit: typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably true, i think it was originally used to prevent the samba process itself to block other network related work, so it does not stall itself. I never tested it without renice, that's why i kept it in and found no other way to include it.
So there is no "official" way to have a package depend on a specific config option? How do other packages handle this, say for example if they depend on a kernel mod or specific kernel option?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can depend on kernel mods however there's no good way of depending on options that doesn't result in a unique package to my knowledge. @karlp @hnyman

help
provides commonly used vfs modules
installs:
modules: vfs_btrfs vfs_fruit vfs_shadow_copy vfs_shadow_copy2 vfs_recycle vfs_fake_perms vfs_readonly vfs_cap vfs_offline vfs_crossrename
Copy link
Contributor

Choose a reason for hiding this comment

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

vfs_btrfs should depend on kmod-fs-btrfs?
From what I can tell you only need vfs_shadow_copy2 not vfs_shadow_copy.
https://github.com/samba-team/samba/blob/master/source3/modules/vfs_shadow_copy.c
https://github.com/samba-team/samba/blob/master/source3/modules/vfs_shadow_copy2.c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for vfs_btrfs, will add a config check and i actually did not check for vfs_shadow_copy, but i think that was the samba default output via "smbd -b", so i kept it close to the default.

help
provides extra vfs modules
installs:
modules: vfs_virusfilter vfs_shell_snap vfs_snapper vfs_commit vfs_worm vfs_xattr_tdb vfs_streams_xattr vfs_aio_fork vfs_aio_pthread vfs_linux_xfs_sgid vfs_netatalk vfs_dirsort vfs_fileid
Copy link
Contributor

Choose a reason for hiding this comment

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

vfs_snapper --> Requires snapper (not in repo)
vfs_aio_* --> Not sure we needs those at all as we have optional aio support?
vfs_linux_xfs_sgid --> should depend on kmod-fs-xfs?

default y

config SAMBA4_SERVER_VFSX
bool "enable extendet VFS modules"
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@@ -0,0 +1,296 @@
#
# Copyright (C) 2007-2017 OpenWrt
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't assign copyright to OpenWrt unless you're affiliated, use your own name or omit it.

DEPENDS:=+zlib +libpthread +libpopt +libcap +libtirpc +libnettle +krb5-libs \
+SAMBA4_SERVER_VFSX:libdbus +SAMBA4_SERVER_ACL:acl \
+SAMBA4_SERVER_AVAHI:libavahi-client +SAMBA4_SERVER_AVAHI:avahi-dbus-daemon \
+SAMBA4_SERVER_AD_DC:python-base +SAMBA4_SERVER_AD_DC:krb5-server +SAMBA4_SERVER_AD_DC:libopenssl +SAMBA4_SERVER_AD_DC:libgnutls +SAMBA4_SERVER_AD_DC:libopenldap
Copy link
Contributor

Choose a reason for hiding this comment

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

Are openssl, gnutls and nettle all needed?

Copy link
Contributor Author

@Andy2244 Andy2244 Apr 16, 2018

Choose a reason for hiding this comment

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

Yes all libs are needed for each configuration they are defined, did a step by step compile test.

@@ -0,0 +1,148 @@
#
# Copyright (C) 2018 OpenWrt.org
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, this should also probably be a separate pull request.

@diizzyy
Copy link
Contributor

diizzyy commented Apr 16, 2018

I see, I left some comments but it looks great otherwise. :-)

@Andy2244
Copy link
Contributor Author

@diizzyy Thanks for the review will try to make the changes next weekend or so.

How you want to handle the qemu case? It's basically just a host helper package, it should compile/run as normal package, yet i did not feel i wanted to maintain this as anything other than a host helper, that's why i used the "@broken" trick. I also did not want to mix it with the existing full qemu package, so thats why i ended up this way.

@Andy2244
Copy link
Contributor Author

Andy2244 commented May 2, 2018

@diizzyy ok, updated to 4.8.1 and done the suggested changes.

@Andy2244 Andy2244 force-pushed the samba-4.8 branch 2 times, most recently from de6784d to 69cab9a Compare May 2, 2018 11:08
@diizzyy
Copy link
Contributor

diizzyy commented May 4, 2018

Great, I think the qemu case is valid so it's fine as it is right now.
I'll give it a try in the weekend.

@diizzyy
Copy link
Contributor

diizzyy commented Jul 19, 2018

I personally don't mind but I don't know the official stance on sprinkling systemd files over the place.

@Andy2244
Copy link
Contributor Author

@diizzyy Good point, found a better candidate for openwrt: /proc/sys/kernel/random/boot_id
Will patch the source to use this if machine-id is not found.

@Andy2244
Copy link
Contributor Author

  • replace netbios default with wsdd2 daemon
  • switch to old smbpasswd backend
  • add 'veto files' to smb.conf template
  • revert parallel build change

@diizzyy ok, added your suggested changes and switched from netbios to wsdd2 by default. Give it a try and if you are happy, maybe we can merge this before the 18.06 release?
Will create the corresponding luci-ui PR on the weekend.

@diizzyy
Copy link
Contributor

diizzyy commented Jul 21, 2018

@Andy2244
You got mail

@Andy2244
Copy link
Contributor Author

Andy2244 commented Jul 21, 2018

  • cleanup/adhere to openwrt guidelines, fix grammar

@diizzyy No worries, i'm just glad someone finally had the time reviewing it and all changes applied.

PS: Seems build-bot is acting up again...

@diizzyy
Copy link
Contributor

diizzyy commented Jul 21, 2018

@Andy2244
It's way beyond what I would be capable to accomplish so I can just offer the little knowledge I have.

Yeah, Travis doesn't seem to be happy.

@hnyman @karlp
Can you have a look at this?

@Andy2244
Copy link
Contributor Author

trivial update

  • fix dir / endings
  • update wsdd2

Signed-off-by: Andy Walsh <andy.walsh44+github@gmail.com>
@Andy2244
Copy link
Contributor Author

Andy2244 commented Jul 26, 2018

  • update qemu-userspace to 3.0.0-rc2

PS: yay travis is happy again!

@Andy2244
Copy link
Contributor Author

Andy2244 commented Jul 30, 2018

@diizzyy So whats left for me to-do, to get this merged? Do i need to rally/find additional reviewers?

@jow-
Copy link
Contributor

jow- commented Jul 30, 2018

I'll see no reason to not merge it, so here we go :)

@jow- jow- merged commit 644ff72 into openwrt:master Jul 30, 2018
@Andy2244
Copy link
Contributor Author

@jow- cool thanks, seems my 6 month odyssey is finally over, now i can just hope all the testing was worth it and hopefully this fails only on a few targets :)

@Andy2244 Andy2244 deleted the samba-4.8 branch July 30, 2018 07:39
@val-kulkov
Copy link
Contributor

Some initial feedback without much further testing -- building samba4 with all options enabled on bcm53xx fails due to missing dependencies:

Package samba4-server is missing dependencies for the following libraries:
libgcrypt.so.20
libpam.so.0
Makefile:381: recipe for target '<snip>/source/bin/packages/arm_cortex-a9/packages/samba4-server_4.8.3-1_arm_cortex-a9.ipk' failed

The libpam dependency looks odd, given the "--without-pam" parameter in CONFIGURE_ARGS.

@Andy2244
Copy link
Contributor Author

@val-kulkov
yeah 001-samba-4.4.0-pam.patch and --without-pam should take care of pam, yet samba4 likes to pull in everything it finds...

libgcrypt.so.20 mhh never had this missing lib on my ubuntu/clear linux hosts, have to check where/when it pulls this in. What host distro do you use, maybe i can reproduce this?

@val-kulkov
Copy link
Contributor

More on the same topic:

Package samba4-libs is missing dependencies for the following libraries:
liblber-2.4.so.2
libldap-2.4.so.2
Makefile:380: recipe for target '/home/val/Development/lede/source/bin/packages/arm_cortex-a9/packages/samba4-libs_4.8.3-1_arm_cortex-a9.ipk' failed

My host is Ubuntu 16.04. The target, again, is bcm53xx.

@Andy2244
Copy link
Contributor Author

Thanks, this seems strange the ldap libs should come from +SAMBA4_SERVER_AD_DC:libopenldap. Will try to reproduce this.

@val-kulkov
Copy link
Contributor

Re the libpam dependency. "CONFIG_PACKAGE_libpam=y" in my .config is what might be triggering this dependency. I wish I had a little more time to investigate this further right now... Will try but cannot promise a quick turnaround.

@Andy2244
Copy link
Contributor Author

Andy2244 commented Jul 30, 2018

oki, will also try with CONFIG_PACKAGE_libpam=y, regarding the other missing libs, need to recompile for your target bcm53xx locally arm(mvbeu)/mips(ARxx) still work with all options enabled for me.

@diizzyy
Copy link
Contributor

diizzyy commented Jul 30, 2018

@Andy2244
Usually you can't really test for these dependency issues due to race conditions unfortunately unless you happen to have a full package repo from the start. Samba is a major pain when it comes to dependencies as it tends to pull in whatever it finds even if it's not needed as you've already noticed. :/

@Andy2244
Copy link
Contributor Author

yeah guess we need to add more: +PACKAGE_name:name, still nice to know what else it pulls in.

@val-kulkov
Copy link
Contributor

The addition of "+PACKAGE_libgcrypt:libgcrypt +PACKAGE_libpam:libpam" to line 52 in Makefile solved the issue for me. All five samba4 packages compiled successfully. Mysteriously, there were no complaints about liblber or libldap this time.

@pprindeville
Copy link
Member

Sorry, was at IETF 102. What was the question?

@diizzyy
Copy link
Contributor

diizzyy commented Jul 31, 2018

@pprindeville
It's all ok, Andy managed to figure it out :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants