Support client-only build #494
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 13 commits
February 22, 2017 09:39
libini_config is used only in ipa-getkeytab and it uses
only functions from libini_config-1.1
sh$ objdump -p /usr/sbin/ipa-getkeytab | grep INI_CONFIG
0x00acdc20 0x00 04 INI_CONFIG_1.1.0
There is not any reason ho have dependency for higher version and lower
dependency will allow to build client only on older distributions.
libpopt added pkg-config file in 1.16 but there are still distributions which has older version of library (el6, el7). And new features from libpopt are not used anywhere. Configure should try to detect as much as possible and users should not use workarounds with explicitely enabled variables as parameters e.g. ./configure POPT_LIBS="-lpopt "
The pkg-config files for xmlrpc_c libraries are shipped just in fedora/rhel due to downstream patch. Debian does not have pkg-config files for xmlrpc_c. Therefore we need to fallback to older method of detection XMLRPC_*FLAGS which was reverted by the commit 1e0143c
The gettext provided macro AM_GNU_GETTEXT checks for required header file "libintl.h" and also provide variable with linker flags LTLIBINTL. The detection is more robus an platform independent. It can also detect situation when gettext is not part of glibc and external library is required. This patch simplify detection and improve portability.
It was used to replace value in ipa-opts.socket but it was removed in the commit d05d111
It is required for installation of python modules therefore
it should be checked at aonfigure time.
GEN ipasetup.py
make[3]: Leaving directory `/workdir/freeipa'
cd .; /usr/bin/python setup.py \
"--verbose" \
build \
--build-base "/workdir/freeipa/ipaclient/build"
Traceback (most recent call last):
File "setup.py", line 29, in <module>
from ipasetup import ipasetup # noqa: E402
File "/workdir/freeipa/ipasetup.py", line 20, in <module>
from setuptools.command.build_py import build_py as setuptools_build_py
ImportError: No module named setuptools.command.build_py
"dirsrv/slapi-plugin.h" is unnecessary for build of ipa_pwd. This patch allow us to move DIRSRV to daemon only dependencies
CentOS and rhel are equivalent but configure time detection failed configure: error: IPA platform centos is not supported
Most of autoconf macros has implicit parameters [action-if-found], [action-if-not-found]. They were not used on many places and configure script tried to check result of tests from ac_cv_* variables. Sometimes variable was checked twice and there was error in both cases. (the 2nd check was a dead code) e.g. ac_cv_header_dirsrv_slapi-plugin_h This patch also fixed mixed horizontal tabs and spaces. It also fixes ver long lines which was difficult to read. Resolves: https://fedorahosted.org/freeipa/ticket/6517
The separate file server.m4/unit_tests.m4 were created as a copy of configure.ac and client parts from server.m4/unit_tests.m4 and server parts from configure.ac were removed in file diff tool. Resolves: https://fedorahosted.org/freeipa/ticket/6517
This patch adds new configure time option --enable-server which is enabled by default. If you want to run client-only part of freeIPA then you can run --enable-server=no or --disable-server. Disabling server also disable related lint checks to reduce dependencies for client-only build. Resolves: https://fedorahosted.org/freeipa/ticket/6517
Some packages from client-only build were in server section and vice versa. There are few unused build requirements: autoconf, automake, libtool, gettext-devel because autoreconf is not executes as part of build process. But They might be usefull in downstream when configure related patches are backported. Resolves: https://fedorahosted.org/freeipa/ticket/6517
* the make target client-check was removed as part of build system refactoring 0a17155 * the directory /usr/share/ipa was removed from freeipa-client-common because packge didn't contain and files there * python3 installation of client-only build was fixed as well Resolves: https://fedorahosted.org/freeipa/ticket/6517
|
I'm not able to run autoreconf, it fails with the following error: |
|
On (22/02/17 00:59), Tomas Krizek wrote:
I'm not able to run autoreconf, it fails with the following error:
```
configure.ac:447: error: required file 'init/tmpfilesd/Makefile.in' not found
asn1/Makefile.am: installing './depcomp'
parallel-tests: installing './test-driver'
autoreconf: automake failed with exit status: 1
```
I cannot see such file in git :-(
```
sh$ git clean -fdx
sh$ ls init/
ipa-dnskeysyncd ipa-ods-exporter Makefile.am systemd
```
and it isn't in configure either
```
sh$ grep "/tmpfilesd" configure.ac *.m4
sh$ $echo $?
1
```
LS
|
|
BTW I tested client-only build on fedora24, fedora25, fedora rawhide,
epel7, debian stable, debian testing, debian unstable
|
|
@lslebodn My bad, there was some leftover stuff that Nevertheless, this does work and allows a client only, as well as installing tests with But I'm not acking, because of the controversy with the |
|
There are two reasons we decided on
The final decision has been made. |
Packagers does not have a use case for installation of ipatests with client only build because integration tests require server and unit tests can be executed as part of build process. But tox uses virtualenv and unit test must be installed for execution. This patch add new configure time option to install tests.
|
NACK on 42fb9b1
Either way, this should be handled by a separate PR and not mixed with client-only builds. |
tiran
requested changes
Feb 22, 2017
| [HAVE_LIBPDB=1], | ||
| [AC_MSG_ERROR([Neither libpdb nor libsamba-passdb does have make_pdb_method])], | ||
| [$SAMBA40EXTRA_LIBPATH]) | ||
| AC_MSG_CHECKING($(basename $PYTHON) module setuptools ) |
There was a problem hiding this comment.
Please put this in a separate PR. This is not related to --disable-server.
| -name '*.h' -print dnl | ||
| > po/POTFILES.in && dnl | ||
| cd "${find_start_pwd}"]) | ||
| [find_start_pwd=`pwd` && dnl |
There was a problem hiding this comment.
Don't mix reformatting with new features. Create another PR for cleanups.
| if test "x${IPAPLATFORM}" == "x"; then | ||
| AC_MSG_ERROR([unable to find ID variable in /etc/os-release]) | ||
| fi | ||
| if test -r "/etc/os-release"; then |
There was a problem hiding this comment.
Don't mix reformatting with new features. Create another PR for cleanups.
| else | ||
| AC_MSG_RESULT([yes]) | ||
| fi | ||
| AC_MSG_CHECKING([if source directory is a Git reposistory]) |
There was a problem hiding this comment.
Don't mix reformatting with new features. Create another PR for cleanups.
| [Vendor string used by package system, e.g. "-1.fc24"]), | ||
| [VENDOR_SUFFIX=${withval}], | ||
| [VENDOR_SUFFIX=""]) | ||
| [AS_HELP_STRING([--with-vendor-suffix=STRING], |
There was a problem hiding this comment.
Don't mix reformatting with new features. Create another PR for cleanups.
| fi | ||
| ]) | ||
| AC_ARG_ENABLE([more-warnings], | ||
| [AC_HELP_STRING([--enable-more-warnings], |
There was a problem hiding this comment.
Don't mix reformatting with new features. Create another PR for cleanups.
| @@ -843,20 +856,25 @@ pushd %{_builddir}/freeipa-%{version}-python3 | |||
| (cd ipalib && %make_install) | |||
| (cd ipaplatform && %make_install) | |||
| (cd ipapython && %make_install) | |||
| %if ! %{ONLY_CLIENT} | |||
There was a problem hiding this comment.
Why is --without-ipatests required at all when you don't install ipatests in ONLY_CLIENT builds any way? This shows that packaging is easily possible without the new argument.
CC @simo5
| @@ -1 +1,3 @@ | |||
| include $(top_srcdir)/Makefile.python.am | |||
|
|
|||
| EXTRA_DIST=centos | |||
configure.ac
Outdated
| fi | ||
|
|
||
| AC_ARG_WITH([tests], | ||
| [AC_HELP_STRING([--with-tests], |
configure.ac
Outdated
| [Whether to install ipatests. ] | ||
| [Default inherited from --disable-server])], | ||
| [with_tests=$withval], | ||
| [with_tests=$enable_server]) |
There was a problem hiding this comment.
NACK, default to true, not $enable_server.
lslebodn
force-pushed
the
enable_server
branch
from
aece4c3
to
f859c3d
Compare
|
On (22/02/17 02:16), Christian Heimes wrote:
NACK on 42fb9b1
* Either use ```--with-ipaplatform=redhat``` on CentOS
* Or implement a proper way to fill ipaplatfrom from ```/etc/os-relase``` value ```ID_LIKE```, https://www.freedesktop.org/software/systemd/man/os-release.html
ID_LIKE is multivalue on centos; it cannot be used.
```
sh# cat /etc/os-release
NAME="CentOS Linux"
VERSION="7 (Core)"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="7"
PRETTY_NAME="CentOS Linux 7 (Core)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:centos:centos:7"
HOME_URL="https://www.centos.org/"
BUG_REPORT_URL="https://bugs.centos.org/"
CENTOS_MANTISBT_PROJECT="CentOS-7"
CENTOS_MANTISBT_PROJECT_VERSION="7"
REDHAT_SUPPORT_PRODUCT="centos"
REDHAT_SUPPORT_PRODUCT_VERSION="7"
```
Either way, this should be handled by a separate PR and not mixed with client-only builds.
The purpose of client only build is to make life of packars simpler.
This patch improves UX so it need to be part of this PR.
LS
|
|
On (22/02/17 02:23), Christian Heimes wrote:
tiran requested changes on this pull request.
see comments
> -CFLAGS="$bck_cflags"
-
-LIBPDB_NAME=""
-AC_CHECK_LIB([samba-passdb],
- [make_pdb_method],
- [LIBPDB_NAME="samba-passdb"; HAVE_LIBPDB=1],
- [LIBPDB_NAME="pdb"],
- [$SAMBA40EXTRA_LIBPATH])
-
-if test "x$LIB_PDB_NAME" = "xpdb" ; then
- AC_CHECK_LIB([$LIBPDB_NAME],
- [make_pdb_method],
- [HAVE_LIBPDB=1],
- [AC_MSG_ERROR([Neither libpdb nor libsamba-passdb does have make_pdb_method])],
- [$SAMBA40EXTRA_LIBPATH])
+AC_MSG_CHECKING($(basename $PYTHON) module setuptools )
Please put this in a separate PR. This is not related to --disable-server.
refactoring/cleaning is requred for minimising dependencies before split.
Otherwise git log would be confusing.
+AM_CONDITIONAL([ENABLE_SERVER], [test x"$enable_server" = xyes])
+if test x"$enable_server" = xyes; then
+ m4_include([server.m4])
+fi
+
+AC_ARG_WITH([tests],
+ [AC_HELP_STRING([--with-tests],
NACK, ```without-ipatests```
already changed.
LS
|
|
On (22/02/17 02:09), Christian Heimes wrote:
There are two reasons we decided on ```--without-ipatests```:
* ```--with-tests``` / ```--without-tests``` is technically not correct. We still compile C tests. The flag is about the component ```ipatests```, so let's call it ```--without-ipatests```.
* ```--with-ipatests``` / ```--without-ipatests``` is only relevant for downstream packaging to make the life of a packager a bit easier. FreeIPA is an upstream first project. The default settings for configure should be convenient and user-friendly for upstream developers and users.
`without-tests` was changed to `without-ipatests`
freeip-4.4 has a weird build system and all downstream packages
had to do many tricks/workaround to install it an package.
The intention of build-refactoring was to make packaging
as simple as possible.
The purpose of client only build
https://fedorahosted.org/freeipa/ticket/6517
Is to allow package just client parts on distriutions which
does not have systemd or they do not want to depend on systemd.
Because ipa-client install just configure sssd, certmonger
which still can be compiled without systemd support.
So the `--disable-server` must disable all parts which requires
anything with server dependencies. Therefore it disable js-lint,
pylint and installation of ipatest. There is a still possiblility
to enable them with client-only build.
e.g. `./configure --disable-server --with-ipatests --enable-pylint`
The final decision has been made.
The decission was made that there will be `--without-ipatests` for tox
use-case. Becasue tox use-case is not a client only build. Therefore
explicit enabling `ipatests` is required for tox use-case.
LS
|
|
You assumption is incorrect. Only some subcomponents of Regarding pylint and jsl, neither of the components should be a build requirement. But that's off-topic for this PR. Please discuss the matter in https://fedorahosted.org/freeipa/ticket/6604 . |
|
On (22/02/17 02:51), Christian Heimes wrote:
You assumption is incorrect. ```ipatests``` does not depend on ```ipaserver```, https://github.com/freeipa/freeipa/blob/master/ipatests/setup.py#L61
```
install_requires=[
"cryptography",
"dnspython",
"gssapi",
"ipaclient",
"ipalib",
"ipaplatform",
"ipapython",
"nose",
"polib",
"pyldap",
"pytest",
"pytest_multihost",
"python-nss",
"six",
],
```
Only some subcomponents of ```ipatests``` do depend on the ```ipaserver``` package or a running server for integration tests, https://github.com/freeipa/freeipa/blob/master/ipatests/setup.py#L77
```
extras_require={
"integration": ["dbus-python", "pyyaml", "ipaserver"],
"ipaserver": ["ipaserver"],
"webui": ["selenium", "pyyaml", "ipaserver"],
"xmlrpc": ["ipaserver"],
}
```
Packagers can run unit tests in-tree. And that's a usual way
how packagers run unit tests.
e.g.
```
PYTHONPATH=$PWD/ \
$PYTHON ./ipatests/ipa-run-tests -vvv --tb=native \
$PWD/ipatests/test_ipaclient/ \
$PWD/ipatests/test_ipalib \
$PWD/ipatests/test_ipapython \
$PWD/ipatests/test_util.py \
$PWD/ipatests/util.py
```
Tox is a special case.
Therefore installation of tests is disabled for `--disable-server`
But for tox it is possible to overrride it.
e.g. `./configure --disable-server --with-tests`
LS
|
|
You are aware that your example code checks the wrong code? It is testing in-tree sources, not the actual sources that get packaged and installed. |
|
On (22/02/17 03:04), Christian Heimes wrote:
You are aware that your example code checks the wrong code? It is testing in-tree sources, not the actual sources that get packaged and installed.
Yes, because unit tests are not usually installed with package.
e.g. `rpm -ql python3-requests | grep tests`
and unit tests are executed as part of build
http://pkgs.fedoraproject.org/cgit/rpms/python-requests.git/tree/python-requests.spec#n158
And I know that your use-case is different.
Therefore there is a configure time option `--with-ipatests`
LS
|
|
python-requests is a bad example because it suffers from the same issue as IPA. A better example is any other modern Python project like cryptography. It runs tests with installed files, not in-tree files. |
|
On (22/02/17 03:24), Christian Heimes wrote:
python-requests is a bad example because it suffers from the same issue as IPA.
A better example is any other modern Python project like cryptography. It runs tests with installed files, not in-tree files.
I check few other quite new projects which were written by RH python guys.
https://admin.fedoraproject.org/pkgdb/package/rpms/devassistant/
https://admin.fedoraproject.org/pkgdb/package/rpms/python-pytest-multihost/
They run unit tests as part of build process and unit tests are not installed.
But maybe I was not just lucky enough to find modern Python project.
Anyway `ipatests` are installed by default with freeipa.
If you want to use non-defalt option for client-only build
then it is possible to install `ipatests` as well.
Thank you for your comments.
LS
|
|
On (22/02/17 03:24), Christian Heimes wrote:
python-requests is a bad example because it suffers from the same issue as IPA.
A better example is any other modern Python project like cryptography. It runs tests with installed files, not in-tree files.
hmm; I probably missed something.
```
sh$ rpm -ql rpm -ql python3-cryptography | grep test
/usr/share/doc/python3-cryptography/docs/development/test-vectors.rst
```
```
sh$ wget --content-disposition https://github.com/pyca/cryptography/archive/1.7.2.tar.gz
2017-02-22 14:10:00 (9.86 MB/s) - ‘cryptography-1.7.2.tar.gz’ saved [27131190]
sh$ tar -xzf cryptography-1.7.2.tar.gz
sh$ find cryptography-1.7.2/ -name "*test*"
cryptography-1.7.2/vectors/cryptography_vectors/keywrap/kwtestvectors
cryptography-1.7.2/vectors/cryptography_vectors/hashes/whirlpool/iso-test-vectors.txt
cryptography-1.7.2/vectors/cryptography_vectors/asymmetric/Traditional_OpenSSL_Serialization/testrsa.pem
cryptography-1.7.2/vectors/cryptography_vectors/asymmetric/Traditional_OpenSSL_Serialization/testrsa-encrypted.pem
cryptography-1.7.2/vectors/cryptography_vectors/asymmetric/DER_Serialization/testrsa.der
cryptography-1.7.2/tests
cryptography-1.7.2/tests/test_x509_revokedcertbuilder.py
cryptography-1.7.2/tests/test_x509_ext.py
cryptography-1.7.2/tests/test_x509_crlbuilder.py
cryptography-1.7.2/tests/test_x509.py
cryptography-1.7.2/tests/test_warnings.py
cryptography-1.7.2/tests/test_utils.py
cryptography-1.7.2/tests/test_interfaces.py
cryptography-1.7.2/tests/test_fernet.py
cryptography-1.7.2/tests/test_cryptography_utils.py
cryptography-1.7.2/tests/hypothesis/test_padding.py
cryptography-1.7.2/tests/hypothesis/test_fernet.py
cryptography-1.7.2/tests/hazmat/primitives/twofactor/test_totp.py
cryptography-1.7.2/tests/hazmat/primitives/twofactor/test_hotp.py
cryptography-1.7.2/tests/hazmat/primitives/test_x963kdf.py
cryptography-1.7.2/tests/hazmat/primitives/test_x963_vectors.py
cryptography-1.7.2/tests/hazmat/primitives/test_serialization.py
cryptography-1.7.2/tests/hazmat/primitives/test_seed.py
cryptography-1.7.2/tests/hazmat/primitives/test_scrypt.py
cryptography-1.7.2/tests/hazmat/primitives/test_rsa.py
cryptography-1.7.2/tests/hazmat/primitives/test_pbkdf2hmac_vectors.py
cryptography-1.7.2/tests/hazmat/primitives/test_pbkdf2hmac.py
cryptography-1.7.2/tests/hazmat/primitives/test_padding.py
cryptography-1.7.2/tests/hazmat/primitives/test_keywrap.py
cryptography-1.7.2/tests/hazmat/primitives/test_kbkdf_vectors.py
cryptography-1.7.2/tests/hazmat/primitives/test_kbkdf.py
cryptography-1.7.2/tests/hazmat/primitives/test_idea.py
cryptography-1.7.2/tests/hazmat/primitives/test_hmac_vectors.py
cryptography-1.7.2/tests/hazmat/primitives/test_hmac.py
cryptography-1.7.2/tests/hazmat/primitives/test_hkdf_vectors.py
cryptography-1.7.2/tests/hazmat/primitives/test_hkdf.py
cryptography-1.7.2/tests/hazmat/primitives/test_hashes.py
cryptography-1.7.2/tests/hazmat/primitives/test_hash_vectors.py
cryptography-1.7.2/tests/hazmat/primitives/test_ec.py
cryptography-1.7.2/tests/hazmat/primitives/test_dsa.py
cryptography-1.7.2/tests/hazmat/primitives/test_dh.py
cryptography-1.7.2/tests/hazmat/primitives/test_constant_time.py
cryptography-1.7.2/tests/hazmat/primitives/test_concatkdf.py
cryptography-1.7.2/tests/hazmat/primitives/test_cmac.py
cryptography-1.7.2/tests/hazmat/primitives/test_ciphers.py
cryptography-1.7.2/tests/hazmat/primitives/test_cast5.py
cryptography-1.7.2/tests/hazmat/primitives/test_camellia.py
cryptography-1.7.2/tests/hazmat/primitives/test_blowfish.py
cryptography-1.7.2/tests/hazmat/primitives/test_block.py
cryptography-1.7.2/tests/hazmat/primitives/test_asym_utils.py
cryptography-1.7.2/tests/hazmat/primitives/test_arc4.py
cryptography-1.7.2/tests/hazmat/primitives/test_aes.py
cryptography-1.7.2/tests/hazmat/primitives/test_3des.py
cryptography-1.7.2/tests/hazmat/bindings/test_openssl.py
cryptography-1.7.2/tests/hazmat/bindings/test_commoncrypto.py
cryptography-1.7.2/tests/hazmat/backends/test_openssl.py
cryptography-1.7.2/tests/hazmat/backends/test_multibackend.py
cryptography-1.7.2/tests/hazmat/backends/test_commoncrypto.py
cryptography-1.7.2/tests/hazmat/backends/test_backendinit.py
cryptography-1.7.2/tests/conftest.py
cryptography-1.7.2/docs/development/test-vectors.rst
```
and unit test are exeuted as part of rpm-build.
```
http://pkgs.fedoraproject.org/cgit/rpms/python-cryptography.git/tree/python-cryptography.spec#n133
```
|
|
On (22/02/17 01:52), Tomas Krizek wrote:
@lslebodn My bad, there was some leftover stuff that `git clean -dfx` didn't clear for some reason.
Nevertheless, this does work and allows a client only, as well as installing tests with `--with-tests` option. The mock build when run with `--without=server` does install less dependencies.
But I'm not acking, because of the controversy with the `--with-tests` option (see #364).
@tomaskrizek FYI `rpmbuild` accepts also parameter `--without server` but it is
not simple to pass it through `make rpms` and it would not check minimal
dependencies in spec file.
LS
|
|
#364 was pushed. |
|
+ack on the xmlrpc-c detection patch at least, I need that on Debian |
|
sorry, I just saw PR#600 which is a subset of this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
How to test: