Build system cleanup phase 2 #171
Conversation
pspacek
force-pushed
the
build-cleanup
branch
from
fc2f30d
to
7805592
Compare
|
This version fixes minor problems in error reporing and should have no other visible effect. |
| AC_CHECK_HEADER(popt.h, [], [AC_MSG_ERROR([popt.h not found])]) | ||
| AC_CHECK_LIB(popt, poptGetContext, [POPT_LIBS="-lpopt"]) | ||
| AC_SUBST(POPT_LIBS) | ||
| PKG_CHECK_MODULES([POPT], [popt]) |
There was a problem hiding this comment.
Older version of libpopt does not provide pkg-config file.
If you want to use PKG_CHECK_MODULES anyway then it would be good to fall back to older style of detection.
sh$ rpm -q popt-devel
popt-devel-1.13-16.el7.x86_64
sh$ pkg-config --list-all | grep pop
There was a problem hiding this comment.
It prints nice informational message how to work it around and it actually works on RHEL 7, so I'm not going to comlicate the build system for crappy packages.
| AC_MSG_ERROR([curl not found]) | ||
| fi | ||
| AC_SUBST(CURL_LIBS) | ||
| PKG_CHECK_MODULES([CURL], [libcurl]) |
There was a problem hiding this comment.
Does it make sense to modernize detection of unused library?
I cannot see include for any curl header file.
$git grep -E "curl/"
client/configure.ac:AC_CHECK_HEADER(curl/curl.h, [], [AC_MSG_ERROR([curl/curl.h not found])])
There was a problem hiding this comment.
Is removed in later patch. I'm not going to rebase this again. Thanks for understanding.
|
ACK to It would be good to push them if you do not want to wait for review of other patches. |
|
ACK, works for me. |
|
-ACK, |
|
Justification is above, please push. |
|
+1 to push, the comments were added to outdated diffs so I thought them resolved. They are now. |
|
I am sorry I still do not agree with popt change reducing 3lines to 1 is not huge simplification. |
|
and the explanation for changing curl is not appropriate. |
|
BTW. |
|
I consider CURL topic to be just bikesheding. Ad POPT: RHEL is going to require explicit configuration as designed in http://www.freeipa.org/page/V4/Build_system_refactoring because you have to explicitly disable missing checks etc. anyway. At the same time you can provide correct POPT flags. If RHEL packager disagrees we can use some auto-magic but I would rather avoid it whenever possible. For reasons above, I think that upstream version should use pkgconfig as it is preferred way for library auto-detection. |
|
@lslebodn If you read the pull request description you will notice that client-only build will be solved later on. |
|
I see just bikeshading here, on IRC Petr1 agreed that this should be pushed and incrementally upgraded according our needs |
pspacek
force-pushed
the
build-cleanup
branch
2 times, most recently
from
96661e7
to
a7690fb
Compare
|
I see you need to increase patch_count and not to have proper review |
|
BTW I am really interested in how do you plan to implement client only build. IMHO it would be much simpler include reduced version of daemon/configure.ac (+others) |
|
@lslebodn Decision if we need client_only build is still open. We may drop it so reimplementing it would be busy work. |
|
My proposal would not require any reimplementation because it will reuse current solution. |
|
@lslebodn Please discuss this matter on freeipa-devel list to gethigher visibility. Please note that current client-only build is quite broken and requires heavy machinery in downstream spec file so it is hard to speak about any reusage. |
This is a pure upstream discussion and how to make at least part of freeIPA more portable rather then closing doors for other platforms/distributions without systemd. There are still people on debian which does not use systemd and want to use sssd and ipa-client. The current patch-set would not create more portable client code. It would would just make it much harder and would require more downstream patches which is not very upstream friendly approach. @pspacek, it would be good if you could comment my proposal with including configure snippets from subdirectories. |
|
I agree with @lslebodn . It should be possible to build only the necessary bits and pieces for an IPA client. It should be rather simple to implement a To be clear: I'm fine with this PR. client-only installation can be added in a later PR. Since it it easy to implement we can re-add it later without much trouble. |
|
ACK make rpms is passing on my machine. |
Currently it is possible to build just a client part with following steps. Ant it has nothing to do with downstream specfile As you can see configure script is executed only in client sub-directory. But after merging everything to the one big configure script it would not be possible And you also wanted to see some patches with nicer solution Or If you you can merge configure part into main configure script rather then |
Translations are need for client as well. This move is done to remove dependency between client and install subdirectories. https://fedorahosted.org/freeipa/ticket/6418
FreeIPA moved to Zanata a while ago. References to Transifex were just leftovers. https://fedorahosted.org/freeipa/ticket/6418
Use package config instead of checking headers. Package config is faster because it does not invoke compiler and guarantees proper linking flags because these are provided by package maintainer instead of hardcoded into build system. https://fedorahosted.org/freeipa/ticket/6418
Use package config instead of checking headers. Package config is faster because it does not invoke compiler and guarantees proper linking flags because these are provided by package maintainer instead of hardcoded into build system. https://fedorahosted.org/freeipa/ticket/6418
Use package config instead of checking headers. Package config is faster because it does not invoke compiler and guarantees proper linking flags because these are provided by package maintainer instead of hardcoded into build system. https://fedorahosted.org/freeipa/ticket/6418
Use package config instead of checking headers. Package config is faster because it does not invoke compiler and guarantees proper linking flags because these are provided by package maintainer instead of hardcoded into build system. https://fedorahosted.org/freeipa/ticket/6418
Explicit check for symbols is unncecessary because libini_config >= 1.2.0 contains all the symbols we need. https://fedorahosted.org/freeipa/ticket/6418
It turns out that default error handling prints very useful messages which are way better than hand-made error handling without any hints. https://fedorahosted.org/freeipa/ticket/6418
This is temporary workaround. makeapi/makeaci transitively import ipaplatform, which will not exist before configure is executed. On the other hand, configure requires version.m4, which is generated by current Makefile. This change works around this chicked-egg problem. It will disappear when we start using top-level configure to generate top-level Makefile. https://fedorahosted.org/freeipa/ticket/6418
This is temporary workaround necessary until we throw away the hand-made Makefile. ipaplatform is going to be managed by configure, but configure right now depends on version-update target in Makefile. https://fedorahosted.org/freeipa/ticket/6418
The original approach with __path__ implemented by 8f98fa1 broke Pylint: We decided to resort back to symlinks as it is easiest solution which does not break pylint in weird ways. This commit introduces configure --with-ipaplatform option. https://fedorahosted.org/freeipa/ticket/6418
Apparently, the docs were not updated when ipapython/platform was moved to ipaplatform module and internals have changed. https://fedorahosted.org/freeipa/ticket/6418
pspacek
force-pushed
the
build-cleanup
branch
from
d92fb98
to
5a40847
Compare
|
@lslebodn I'm really trying to explain this but I'm still not able to get the point across.
Please understand that IPA client consists of components in C as well from components in Python, and also non-programatic components like translation machinery etc. We certainly can create m4 include maze and force maintainer to always use The end goal is to fold all of hand-made Makefile and SPEC file scripts to the build system, so in the end, it should help with porting to other arches - there will be just one place where changes need to be done, instead of three. I hope that it clears up why it is not useful to insist on keeping current pieces as they were before. The design document was sent to freeipa-devel mailing list in this thread: Thank you for understanding. |
|
This PR including additional patches on top have passed build + all XMLRPC tests in Jenkins: Additional commits can be found on The test included everything up to e33e00b |
|
On (20/10/16 10:29), Petr Špaček wrote:
Correct me If I am wrong. The configure script is and will be used So here I cannot see a conflict with my proposal.
I cannot see a reason why it would be a maze There would not be much includes as I showed in POC
The most common use case is just to run configure script and However, C related code is not changed very often and even more
Totally agree. But python components is not related to checking of
Agree, but here I cannot see any conflict with my proposal.
I read the desing page and there is mentioned that autotools I could not find any contradiction between my proposal and desing page. LS |
Maybe this is why we do not understand each other. Autotools will orchestrate the complete build including configuration of Python (and other) components. This is what I meant by This PR starts to use configure to create symlinks in ipaplatform Python module and more things are comming. If you look at http://www.freeipa.org/page/V4/Build_system_refactoring#Configuration you will see that configure is going contain switches for Python components as well as for Javascript components. I.e. there is nothing like C-only configure anymore. I hope it clears the intent. |
|
On (21/10/16 00:41), Petr Špaček wrote:
I cannot see a problem here. Design page just say that you will be able to optionally build and install
Design page says something else. My intention is to have a simple way how to implement "--enable-server/--disable-server". Merging everything to the one big configure script will not simplify it. It will just complicate it. And the purpose of refactoring is to make things simpler and easily maintainable. |
|
We (lslebodn and pspacek) agree that we disagree about maintainability of client_only in one monolithic configure.ac. But we agreed that the refactoring of build system[1] is more important. I will close my eyes and you can push the patch-set. :-) |
|
Re-adding ACK which was removed while we were "resolving" our disagreement with Lukas. |
ghost
closed this
|
On (20/10/16 10:29), Petr Špaček wrote:
So here I cannot see a conflict with my proposal.
There would not be much includes as I showed in POC
However, C related code is not changed very often and even more
I could not find any contradiction between my proposal and desing page. LS |
This merges all the configure scripts into one modern, including client. The end state will be one configure script with appropriate options which will allow to build only client as necessary.
Client-only build does not work right now so it broke ONLY_CLIENT option in SPEC file. I'm sending it early so we can get review and merge it sooner rather than later.