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

Build system must regenerate file when template changes #268

Closed
wants to merge 6 commits into from

Conversation

pspacek
Copy link
Contributor

@pspacek pspacek commented Nov 23, 2016

Proper fix for https://fedorahosted.org/freeipa/ticket/6498.

This PR obsoletes #251.

AC_CONFIG_FILES in configure.ac works well only with Makefiles.
Other files have to be handled by Makefile.am so depedencies
are tracked properly.

https://fedorahosted.org/freeipa/ticket/6498
AC_CONFIG_FILES in configure.ac works well only with Makefiles.
Other files have to be handled by Makefile.am so depedencies
are tracked properly.

https://fedorahosted.org/freeipa/ticket/6498
AC_CONFIG_FILES in configure.ac works well only with Makefiles.
Other files have to be handled by Makefile.am so depedencies
are tracked properly.

https://fedorahosted.org/freeipa/ticket/6498
AC_CONFIG_FILES in configure.ac works well only with Makefiles.
Other files have to be handled by Makefile.am so depedencies
are tracked properly.

https://fedorahosted.org/freeipa/ticket/6498
AC_CONFIG_FILES in configure.ac works well only with Makefiles.
Other files have to be handled by Makefile.am so depedencies
are tracked properly.

There is a problem that Python sub-directories depend on ipasetup.py
which is one level above the sub-directory. This means that depedencies
are the other way around that expected. This is being worked around
using hack from
http://lists.gnu.org/archive/html/automake/2009-03/msg00011.html

https://fedorahosted.org/freeipa/ticket/6498
@MartinBasti
Copy link
Contributor

works for me

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Nov 25, 2016
@tiran
Copy link
Member

tiran commented Nov 25, 2016

I don't like the approach and prefer AC_CONFIG_FILE over manual sed for templating. You only have to add a couple of rules like

# Makefile.python.am
$(top_builddir)/ipasetup.py: $(top_builddir)/config.status $(top_builddir)/ipasetup.py.in
        $(MAKE) -C $(top_builddir) $(@F)

@pspacek
Copy link
Contributor Author

pspacek commented Nov 25, 2016

I already described problems with AC_CONFIG_FILE in #251 (comment) a week ago, including envisioned move from AC_CONFIG_FILE to Makefile.am.

Please propose a solution which does not have problems mentioned in #251 (comment) so we can consider it.

For the record, this sed replacement is nothing unusual. The sed replacement is what Autoconf v2.69 manual chapter 4.8.2 Installation Directory Variables recommends and is already used all over the place in the build system (init directory, daemons/ipa-otpd, and elsewhere).

It is pitty that you did not comment on envisioned direction a week ago, nor a three days ago when first version of this PR was published.

@tiran
Copy link
Member

tiran commented Nov 25, 2016

You gave a good reason to not use CONFIG_STATUS_DEPEDENCIES and I agree with your reasoning. I don't see a case against AC_CONFIG_FILE, though. config.status substitution feature is more powerful than manual sed rules. I'm worried that we are going to run into problems in the future. It's surprising that some files can use all @VAR@ substitutions and some only a limited subset.

Your patch already introduces proper dependencies for ipasetup.py and version.py. Why not introduce a build rule for these files in Makefile.python.am?

@pspacek
Copy link
Contributor Author

pspacek commented Nov 25, 2016

Oh, you are right, I was mixing CONFIG_STATUS_DEPEDENCIES and AC_CONFIG_FILES. Sorry!

So please let me explain the problem with AC_CONFIG_FILES:
AC_CONFIG_FILES properly substitutes variables only in Makefiles, as explained in Autoconf v2.69 manual chapter 4.8.2 Installation Directory Variables.

Yes, we can use AC_CONFIG_FILES so all variables can used for substitution, but in that case only subset of all usable variables will be substituted correctly. I do not think that it is right approach.

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Nov 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants