Build phase 7: cleanup #236
Conversation
| @@ -30,26 +27,22 @@ AM_CPPFLAGS = \ | |||
| $(SASL_CFLAGS) \ | |||
| $(POPT_CFLAGS) \ | |||
| $(WARN_CFLAGS) \ | |||
| $(INI_CFLAGS) \ | |||
| $(NULL) | |||
| $(INI_CFLAGS) | |||
|
|
|||
There was a problem hiding this comment.
The reason why is $(NULL) is at the end of list is simpler diff when adding new entries.
If there is not NULL at the end and you want to add new entry there then diff will contain 1 removed line + 2 added lines
If there is NULL at the end and you want to add new entry there then diff will contain just 1 added line.
|
You should the opposite of the last patch "Build: clean-up spurious NULL variables from Makefile.am files". The NULL should be added to each list. NACK to the last patch. It does not simplify anything and it makes diff more complicated for adding new entries |
| @@ -2,13 +2,10 @@ | |||
|
|
|||
| AUTOMAKE_OPTIONS = 1.7 subdir-objects | |||
|
|
|||
| NULL = | |||
|
|
|||
There was a problem hiding this comment.
These lines can be removed because NULL needn't be defined.
|
Hi Lukas. Given there is no technical justification to have it I'm going to remove these. Simple is better than complex. |
|
| # run configure with the same parameters as RPM build | ||
| # this makes it easy to tweak files locally and use make install | ||
| test ! -f "Makefile" && ./configure --enable-silent-rules \ | ||
| --host=$(rpm -E %{_host}) \ |
There was a problem hiding this comment.
NACK
rpm should be an optional dependency to support platforms without rpm command.
There was a problem hiding this comment.
@tiran can you explain how a script with the name makerpms.sh cannot be rpm specific?
pspacek
force-pushed
the
build-phase-7
branch
from
42804d5
to
28b692b
Compare
|
@lslebodn I've dropped the controversial patch which removes NULLs to allow you to send PR which adds it everywhere as you proposed. I'm going to wait till deadline at end of this working week. If your PR is not revied by then I'm going to push the removal so things are at least consistent. |
|
Thank you very much for removing problematic patch. Do not forget to remove it from "Build phase 8" :-) as well A) there are few conflicts => patch need to be rebased B) I do not think that @tiran objection about rpm in makerpms.sh is valid but he should reply. Otherwise ACK after rebase BTW is there a plan to have "make dictcheck" functional? |
Setuptools will print only warnings. The option has to be used before setuptools command specification, otherwise it will not apply to sub-commands. https://fedorahosted.org/freeipa/ticket/6418
Build called from makerpms.sh is not verbose by default anymore. It still prints all directories and files it builds but the long command lines are hidden by default. It has the advantage that compiler and other warnings are visible to developers right away. If you need to debug something, use --disable-silent-rules to override the default (or call configure manually). https://fedorahosted.org/freeipa/ticket/6418
Automake manual section 13 What Gets Cleaned says that make maintainer-clean should not remove files necessary for subsequent runs of ./configure. It practically means that all usage of MAINTAINERCLEANFILES were incorrect so I've removed them. https://fedorahosted.org/freeipa/ticket/6418
This allows us to simply use makerpms.sh to configure the build tree, install RPMs to configure system for the first time and then use make install for rapid devel/test cycles. Configuration parameteres were taken from rpm-4.13.0-0.rc1.27.fc24.x86_64. https://fedorahosted.org/freeipa/ticket/6418
Some of .less files included by ipa.less were not listed in the Makefile.am so some changes might not trigger rebuild. https://fedorahosted.org/freeipa/ticket/6418
pspacek
force-pushed
the
build-phase-7
branch
from
28b692b
to
5a5fab2
Compare
|
Rebased. |
|
@lslebodn There is currently no plan to support distcheck: Python setuptools do not support VPATH builds as AFAIK it is impossible to do that without patching setuptools heavily. |
|
ACKing on behalf of Lukas. |
Depends on PR #233.