Client-only builds with --disable-server #364

tiran · 2017-01-03T13:40:11Z

https://fedorahosted.org/freeipa/ticket/6517

Simple approach to reduce dependencies for client only builds. I simply wrapped all daemon, init and install dependencies in AM_COND_IF(). I'm open to suggestions. Let the bike shedding begin!

tkrizek · 2017-01-11T13:44:02Z

Could you please extend V4/Build system refactoring to include steps describing how to perform client-only build?

Also, is this supposed to build the freeipa-client rpm package? I wasn't able to build the rpm ( *** No rule to make target 'distdir'. Stop.).

Or does it only support make + make install? When I tried to do this it also produced ipaserver and ipatests directories. It was probably a misconfiguration on my side.

Please provide instructions on how to do a proper client-only build.

tiran · 2017-01-11T14:18:31Z

The PR only affects make install, Python packaging and integration efforts. The goal is to reduce the amount of necessary build dependencies for installation and packaging of Python wheels or for developers that are only interested to build ipa-client locally.

At the moment it is not possible to build FreeIPA without Samba, talloc, tevent, 389 DS, systemd and a couple of more packages. The dependency tree is rather heavy. These dependencies are not relevant for clients, though.

I haven't touched RPM builds deliberately. RPM spec can be adjusted in a subsequent PR. It's not relevant for me.

tkrizek · 2017-01-11T15:19:35Z

The extra dependencies are indeed not necessary with this change.

However, make install produces directories like /usr/lib/python2.7/site-packages/ipaserver, /usr/lib/python2.7/site-packages/ipatests, ... I don't think these should be present when doing a client-only build.

tiran · 2017-01-11T15:38:17Z

Nit-pick: A build does not produce any files outside the build environment. Of course make install produces the files -- unless you change the prefix with ./configure --prefix.

tkrizek · 2017-01-17T12:18:25Z

I'm not really experienced with autotools, so I do not want to ack this PR without someone else taking a look. I'm also not sure about the best practices in this area. Perhaps @lslebodn could share his opinion on this change?

lslebodn · 2017-01-17T12:29:22Z

I think it would be simpler to read the code without to many AM_COND_IF(). IMHO it would be simpler to move C-related part to separate file (e.g. server_daemons.m4) an conditionally include the file with m4_include.

I checked it very briefly; I might miss something. How do you want to handle python server part?

tiran · 2017-01-17T12:37:16Z

@lslebodn I like the idea to move the server related header and lib detection to a separate m4 file.

In server-less mode, I plan to ignore the Python server part completely.

lslebodn · 2017-01-17T12:46:17Z

I fine with ignoring python related parts; but it should be documented. But you might ask other freeIPA developers. (maybe on freeipa-devel)

tiran · 2017-01-17T13:05:42Z

@lslebodn I think you misunderstood me. The PR adds a new build flavor that ignores and skips the any server-related steps of the build process. The fact that this PR ignores the server part is actually a a feature. :)

The last version of the patch now skips ipaserver subdir completely, too.

lslebodn · 2017-02-08T16:24:40Z

make dist failed if configure was executed with --disable-server

make[3]: Leaving directory '/workdir/freeipa/po'
make[2]: Leaving directory '/workdir/freeipa/po'
 (cd daemons && make  top_distdir=../freeipa-4.4.90.dev201702081623+git9da17b545 distdir=../freeipa-4.4.90.dev201702081623+git9da17b545/daemons \
     am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory '/workdir/freeipa/daemons'
make[2]: *** No rule to make target 'distdir'.  Stop.
make[2]: Leaving directory '/workdir/freeipa/daemons'
make[1]: *** [Makefile:707: distdir] Error 1
make[1]: Leaving directory '/workdir/freeipa'
make: *** [Makefile:806: dist] Error 2

Do we care?

lslebodn · 2017-02-08T17:46:06Z

Makefile.am

+
+if ENABLE_SERVER
+    SUBDIRS += daemons init install ipaserver
+endif


I think you can move ipatests as well because they require server.

Only some parts of the test suite require ipaserver. I have a branch that can execute just the client tests with tox.

ipatests/test_xmlrpc/ and ipatests/test_integration/ requires them

lslebodn · 2017-02-08T17:46:39Z

See inline comment and issue above; otherwise LGTM.

tkrizek · 2017-02-08T18:10:57Z

@tiran Do you need make dist for anything?

I'm not aware of any plans to release client-only IPA sources, so I don't think it's needed.

tiran · 2017-02-08T18:21:27Z

@tomaskrizek @lslebodn Although I don't need make dist, you made me aware of a bug in Makefile.am. automake and += do not mix well. I moved the list into a new var SERVER_SUBDIRS.

tkrizek · 2017-02-08T18:47:28Z

Makefile.am

 IPACLIENT_SUBDIRS = ipaclient ipalib ipapython
-SUBDIRS = asn1 util client contrib daemons init install $(IPACLIENT_SUBDIRS) ipaplatform ipaserver ipatests po
+SUBDIRS = asn1 util client contrib $(IPACLIENT_SUBDIRS) ipaplatform ipatests po $(SERVER_SUBDIR)


Probably a typo here, missing S at the end of $SERVER_SUBDIR.

lslebodn · 2017-02-08T19:12:56Z

configure.ac

-    install/updates/Makefile
-    install/restart_scripts/Makefile
-    install/wsgi/Makefile
-    install/oddjob/Makefile
    ipaclient/Makefile


This change caused me troubles after fixing typo in SERVER_SUBDIR.
Why did you move them?

autoreconf -if ./configure --disable-server make dist

I moved the makefiles because they use autoconf vars that are not available in --disable-server mode. I see two ways to solve the problem:

still generate all makefiles although some are potentially broken

set DIST_SUBDIRS = $(SUBDIRS)

https://www.gnu.org/software/automake/manual/html_node/SUBDIRS-vs-DIST_005fSUBDIRS.html

tkrizek · 2017-02-09T09:38:03Z

Server build works now, but there's still the make dist issue discussed above.

tiran · 2017-02-09T10:06:52Z

I followed @lslebodn advice and changed the PR a bit. I now generate all Makefiles again to fix the make dist issue. Some of the Makefile are not working correctly because some vars are declared empty (e.g. header locations, libs and so on). Since they are not included in SUBDIRS, they are not used in make. make dist uses DIST_SUBDIRS and does not depend on the missing vars.

pvoborni · 2017-02-10T14:11:38Z

I still think that this use case needs to be documented in http://www.freeipa.org/page/V4/Build_system_refactoring#How_to_Use .

IMHO make dist can fail with --disable-server. Use case for make dist is releasing and there is no point to do release without server bitw. But make sure to document it and test that make dist works without it ;) .

lslebodn · 2017-02-10T14:18:16Z

On (10/02/17 06:11), Petr Vobornik wrote: I still think that this use case needs to be documented in http://www.freeipa.org/page/V4/Build_system_refactoring#How_to_Use . IMHO `make dist` can fail with --disable-server. Use case for `make dist` is releasing and there is no point to do release without server bitw. But make sure to document it and test that `make dist` works without it ;) .

Petr FYI: make dist should include all files into tarball even though configure was invoked with parameter --disable-server. LS

pvoborni · 2017-02-17T09:12:51Z

I still fail to see why we should care about make dist with configure --disable-server this is not a combination of options which should be used together, there is no point in it except theoretical exercise.

then I will need to send PR to fix client-only build and it will not install ipatests with --disable-server.

Why?

lslebodn · 2017-02-17T11:04:19Z

On (17/02/17 01:12), Petr Vobornik wrote: I still fail to see why we should care about `make dist` with `configure --disable-server` this is not a combination of options which should be used together, there is no point in it except theoretical exercise.

`make dist` works

> then I will need to send PR to fix client-only build and it will not install ipatests with --disable-server. Why?

The main problem with `client-only` build and `ipatests` is that there are not test for it. * all integration test assume that server will be installed first and not that server is already available somewhere. * unit test should be run as part of build (e.g. `make check`) So there is not any reason to install `ipatests` for client only build. Client only build was tested just in downstream :-( LS

lslebodn · 2017-02-17T15:11:53Z

It looks like @tiran does not want to move this PR forward. So here is a link to patch which does not misuse client-only build and add configure time option to install ipatests. https://paste.fedoraproject.org/paste/~u7iDljnMTvinxmLFoc25V5M1UNdIGYhyRLivL9gydE=/ As part of this work I found out that 4 dependencies can be simply moved to server only build: `libuuid`, `DIRSRV`, `libsss_idmap`, `libsss_nss_idmap`. Result => Client only build require less C-dependencies. LS

tiran · 2017-02-17T15:53:54Z

Thanks for your contribution. I added your patch to my PR. On my system I ran into a minor issue. Some C99 types like uint8_t were not defined and I had to include stdint.h.

By the way I'm just going to ignore your snidely and snarky comment.

https://fedorahosted.org/freeipa/ticket/6517

"dirsrv/slapi-plugin.h" is unnecessary for build of ipa_pwd. This patch allow us to move DIRSRV to daemon only dependencies

Signed-off-by: Christian Heimes <cheimes@redhat.com>

--without-ipatests skips building and installation of the ipatests package. By default the ipatests package is always build and installed by make install. Signed-off-by: Christian Heimes <cheimes@redhat.com>

tiran · 2017-02-21T20:58:50Z

Now --without-ipatests argument for @lslebodn

simo5

LGTM

HonzaCholasta · 2017-02-22T06:41:23Z

@tiran, not just for @lslebodn, --without-ipatests will be very useful to me for RHEL and Arch Linux packaging as well 😉.

lslebodn · 2017-02-22T08:48:32Z

Thanks for your contribution. I added your patch to my PR. On my system I ran into a minor issue. >Some C99 types like uint8_t were not defined and I had to include stdint.h.

This change is not enough; there is still warning:

ipa_pwd_ntlm.c: In function 'encode_nt_key':
ipa_pwd_ntlm.c:58:5: warning: implicit declaration of function 'strlen' [-Wimplicit-function-declaration]
     il = strlen(newPasswd);
     ^
ipa_pwd_ntlm.c:58:10: warning: incompatible implicit declaration of built-in function 'strlen'
     il = strlen(newPasswd);
          ^

By the way I'm just going to ignore your snidely and snarky comment.

No problem. I am going to forget that my proposal for compromise was ignored for 12 days.

The latest version is a small improvement; but there are still problems/small issues because this PR was created with intention to use tox. I know you are busy. So I wrote client-only implementation from scratch.

This PR is superseded by #494

tkrizek · 2017-02-22T09:54:10Z

The PR works and the --without-ipatests option omits the ipatests directory.

However, #494 doesn't install extra dependencies with mock --without=server.

simo5 · 2017-02-22T14:16:37Z

So this is the reasoning and why I am approving this PR and not #494.

When you build all components, including server bits, tests are installed, therefore when we build just client bits tets that are relevant to client bits also need to be installed for consistency.

Any switch should default to the same behavior regardless of whether server build is enabled. It is confusing if the --with[out]-[ipa]tests switch changes default based on a different switch passed to configure.

As far as I understand this PR maintains the same default for either server or client only builds, so it gets my approval.

lslebodn · 2017-02-22T14:41:39Z

On (22/02/17 06:16), Simo Sorce wrote: So this is the reasoning and why I am approving this PR and not #494. When you build all components, including server bits, tests are installed, therefore when we build just client bits tets that are relevant to client bits also need to be installed for consistency. Any switch should default to the same behavior regardless of whether server build is enabled. It is confusing if the --with[out]-[ipa]tests switch changes default based on a different switch passed to configure. As far as I understand this PR maintains the same default for either server or client only builds, so it gets my approval.

Neither of python packages which I mention in #494 package unit test in fedora. So there is not a reason to package them by default for client only build. And integration tests require server therefore must not be installed by default with client-only build. Result: This PR has wrong default for instalation of ipatests with client-only build. LS

pvoborni · 2017-02-22T14:49:00Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/70554938d4f9ba5b347cd4bc8001428e905198e4
https://fedorahosted.org/freeipa/changeset/41d7ae54fafc6deb602e1a990eaec37c6ae4880b
https://fedorahosted.org/freeipa/changeset/20c1eb9844223d892da47da1ea10662d37953ff8
https://fedorahosted.org/freeipa/changeset/2747f2ad782c7640ecc6949098f0d43411182255

pvoborni · 2017-02-22T14:58:13Z

Also I added section to FreeIPA wiki: http://www.freeipa.org/page/V4/Build_system_refactoring#Packager_-_client_only_build

tiran force-pushed the client-only branch from cdb4397 to bdafd04 Compare January 11, 2017 14:10

tiran force-pushed the client-only branch from bdafd04 to aa6d89d Compare January 17, 2017 13:05

tiran force-pushed the client-only branch 2 times, most recently from 64ff3f9 to ea8c35a Compare January 20, 2017 12:32

tiran force-pushed the client-only branch from ea8c35a to 6953341 Compare February 6, 2017 09:02

lslebodn reviewed Feb 8, 2017

View reviewed changes

tiran force-pushed the client-only branch from 6953341 to 1c44c2a Compare February 8, 2017 18:19

tkrizek suggested changes Feb 8, 2017

View reviewed changes

lslebodn reviewed Feb 8, 2017

View reviewed changes

tiran force-pushed the client-only branch from 1c44c2a to 157cfa0 Compare February 8, 2017 20:06

tkrizek approved these changes Feb 9, 2017

View reviewed changes

tiran force-pushed the client-only branch 2 times, most recently from 7dc6685 to 95026f5 Compare February 9, 2017 10:01

tiran force-pushed the client-only branch from 95026f5 to 4ce2b43 Compare February 17, 2017 15:35

tiran and others added 4 commits February 21, 2017 20:36

Client-only builds with --disable-server

b6f522c

https://fedorahosted.org/freeipa/ticket/6517

ipa_pwd: remove unnecessary dependency on dirsrv plugins

144b1c6

"dirsrv/slapi-plugin.h" is unnecessary for build of ipa_pwd. This patch allow us to move DIRSRV to daemon only dependencies

Add missing include of stdint.h for uint8_t

98988cc

Signed-off-by: Christian Heimes <cheimes@redhat.com>

Add --without-ipatests option

1364d4f

--without-ipatests skips building and installation of the ipatests package. By default the ipatests package is always build and installed by make install. Signed-off-by: Christian Heimes <cheimes@redhat.com>

tiran force-pushed the client-only branch from 4ce2b43 to 1364d4f Compare February 21, 2017 20:57

simo5 approved these changes Feb 21, 2017

View reviewed changes

tkrizek mentioned this pull request Feb 22, 2017

Support client-only build #494

Closed

simo5 added the ack Pull Request approved, can be merged label Feb 22, 2017

pvoborni added the pushed Pull Request has already been pushed label Feb 22, 2017

pvoborni closed this Feb 22, 2017

tiran deleted the client-only branch February 22, 2017 20:58

lslebodn mentioned this pull request Mar 1, 2017

C compilation fixes and hardening #501

Closed

tkrizek mentioned this pull request Mar 1, 2017

Make pylint and jsl optional #502

Closed

Client-only builds with --disable-server #364

Client-only builds with --disable-server #364

tiran commented Jan 3, 2017 •

edited by pvoborni

tkrizek commented Jan 11, 2017

tiran commented Jan 11, 2017

tkrizek commented Jan 11, 2017

tiran commented Jan 11, 2017

tkrizek commented Jan 17, 2017

lslebodn commented Jan 17, 2017

tiran commented Jan 17, 2017

lslebodn commented Jan 17, 2017

tiran commented Jan 17, 2017

lslebodn commented Feb 8, 2017

lslebodn Feb 8, 2017

tiran Feb 8, 2017

lslebodn Feb 8, 2017

lslebodn commented Feb 8, 2017

tkrizek commented Feb 8, 2017

tiran commented Feb 8, 2017

tkrizek Feb 8, 2017

lslebodn Feb 8, 2017

lslebodn Feb 8, 2017 •

edited

tiran Feb 8, 2017

tkrizek commented Feb 9, 2017

tiran commented Feb 9, 2017

pvoborni commented Feb 10, 2017

lslebodn commented Feb 10, 2017 via email

pvoborni commented Feb 17, 2017

lslebodn commented Feb 17, 2017 via email

lslebodn commented Feb 17, 2017 via email

tiran commented Feb 17, 2017

tiran commented Feb 21, 2017 •

edited

simo5 left a comment

HonzaCholasta commented Feb 22, 2017

lslebodn commented Feb 22, 2017 •

edited

tkrizek commented Feb 22, 2017

simo5 commented Feb 22, 2017

lslebodn commented Feb 22, 2017 via email

pvoborni commented Feb 22, 2017

pvoborni commented Feb 22, 2017

Client-only builds with --disable-server #364

Client-only builds with --disable-server #364

Conversation

tiran commented Jan 3, 2017 • edited by pvoborni

tkrizek commented Jan 11, 2017

tiran commented Jan 11, 2017

tkrizek commented Jan 11, 2017

tiran commented Jan 11, 2017

tkrizek commented Jan 17, 2017

lslebodn commented Jan 17, 2017

tiran commented Jan 17, 2017

lslebodn commented Jan 17, 2017

tiran commented Jan 17, 2017

lslebodn commented Feb 8, 2017

lslebodn Feb 8, 2017

Choose a reason for hiding this comment

tiran Feb 8, 2017

Choose a reason for hiding this comment

lslebodn Feb 8, 2017

Choose a reason for hiding this comment

lslebodn commented Feb 8, 2017

tkrizek commented Feb 8, 2017

tiran commented Feb 8, 2017

tkrizek Feb 8, 2017

Choose a reason for hiding this comment

lslebodn Feb 8, 2017

Choose a reason for hiding this comment

lslebodn Feb 8, 2017 • edited

Choose a reason for hiding this comment

tiran Feb 8, 2017

Choose a reason for hiding this comment

tkrizek commented Feb 9, 2017

tiran commented Feb 9, 2017

pvoborni commented Feb 10, 2017

lslebodn commented Feb 10, 2017 via email

pvoborni commented Feb 17, 2017

lslebodn commented Feb 17, 2017 via email

lslebodn commented Feb 17, 2017 via email

tiran commented Feb 17, 2017

tiran commented Feb 21, 2017 • edited

simo5 left a comment

Choose a reason for hiding this comment

HonzaCholasta commented Feb 22, 2017

lslebodn commented Feb 22, 2017 • edited

tkrizek commented Feb 22, 2017

simo5 commented Feb 22, 2017

lslebodn commented Feb 22, 2017 via email

pvoborni commented Feb 22, 2017

pvoborni commented Feb 22, 2017

tiran commented Jan 3, 2017 •

edited by pvoborni

lslebodn Feb 8, 2017 •

edited

tiran commented Feb 21, 2017 •

edited

lslebodn commented Feb 22, 2017 •

edited