configure/Makefile: Makefile.common is not .in anymore#9453
configure/Makefile: Makefile.common is not .in anymore#9453gasche wants to merge 3 commits intoocaml:trunkfrom
Conversation
…e anymore The changes from -include to include are a partial revert of d4a5665 (No change entry needed)
|
I'm just about to do a full review, but just to say that I wholly support this - I got burned just yesterday by updating |
dra27
left a comment
There was a problem hiding this comment.
This is all good. I have one thought, though. Rather than moving these to Makefile.config.in, might it be worth having Makefile.internal-config.in (which would mean keeping the include Makefile.common lines you've restored and restoring the deleted -include with a new name). The issue - and it might be why @shindere created Makefile.common.in in the first place (??) - is that we leak these definitions to anyone who includes the installed Makefile.config on an OCaml system. I expect this is much less common than it used to be (because make isn't used as much for OCaml projects), but I think Sebastien's original hope was to clean up definitions in Makefile.config.
|
I'm okay with the principle, too.
I was actually trying to remember why I put the definition in
`Makefile.common` rather than `Makefile.config` (I think it was me who
did that, initially). I believe that my concern was not to touch a file
that will be installed, to avoid creating compatibility issues, like
people starting to rely on variables we could want to get rid of later.
That does not invalidate the proposed PR, I just would like to make sure
people are aware of this aspect of the question.
Also, regarding the use of configuration files, I think that using them
or not is also a bit a question of personal tastes. I, personnally, tend
to like them, becuse autoconf has already all the machinery in place to
generate them and I find it nicer than our Makefile rules.
|
|
David Allsopp (2020/04/17 09:09 +0000):
@dra27 commented on this pull request.
This is all good. I have one thought, though. Rather than moving these
to `Makefile.config.in`, might it be worth having
`Makefile.internal-config.in` (which would mean keeping the `include
Makefile.common` lines you've restored and restoring the deleted
`-include` with a new name). The issue - and it might be why @shindere
created `Makefile.common.in` in the first place (??) - is that we leak
these definitions to anyone who includes the installed
`Makefile.config` on an OCaml system. I expect this is much less
common than it used to be (because `make` isn't used as much for OCaml
projects), but I think Sebastien's original hope was to clean up
definitions in `Makefile.config`.
It seems you know me quite well, David. ;-)
|
|
The "inclusion in user Makefiles" is a good catch, and I was going to suggest using a longer name, like OCAML_INSTALL_CMD, in Makefile.config, and then rebinding that to our convenient name in Makefile.common. But then, looking at Makefile.config, I see that we keep adding non-namespaced variables, like NATIVE_COMPILER or FUNCTION_SECTIONS added recently. Given that the cat is already out of the bag, I don't see a reason to special-case INSTALL, especially given the convenience benefits of having only one configure-generated file. |
|
|
|
@gasche: this looks like something that could interfere with the dune build. Could you try to |
|
@dra27 @trefis no I haven't looked, sorry. There is no ill will on my part but I never know how to even run the dune build, and my attempts fail with an error 90% of the time, so I'm not the right person to check this. (My merlin workflow is to copy a |
|
Renaming it would certainly work, but it does feel better to move it to another file completely - the information has no purpose being installed. I don't see what the "convenience benefits" of having one configure-generated file are? |
|
I just checked the dune build, it seems to be working fine. If you can lend me some help, I'm willing, this week-end, to try adding a "merlin" makefile rule (that would call dune), add some documentation to HACKING.adoc, and add a travis check that the rule works (that's the part for which I need help). |
|
David Allsopp (2020/04/17 03:22 -0700):
Renaming it would certainly work, but it does feel better to move it
to another file completely - the information has no purpose being
installed. I don't see what the "convenience benefits" of having one
configure-generated file are?
I agree with @dra27.
|
|
@trefis happy to help with that. @dra27 @shindere I cannot tell if you are both suffering from Stockholm syndrome or if I'm the only one who strongly dislikes having to deal with |
|
The thing which confuses me slightly is that you seem to be insisting on conflating two things - there's the developer-benefitting "don't put non-configure related things in a file generated by configure" (i.e. be able to put recipes, common definitions and so forth in Anyway, the merging of #8639 and a variable change in #9332 means that #9332 is now blocked on this PR. #9332 contains a version of this PR which introduces Can we move to a decision on what happens with this PR so that the other can be rebased (if necessary) or just merged. |
|
I think than what you are saying is that what I want is to have only the strictly necessary stuff in This is a good remark and I mostly agree. Note that the difference in handling template makefiles vs. normal files (in gitignore etc.) means that adding new template files also has a cost (a lot of ceremony), so personally my impression is that restricting oneself to just one (minimal) template file would be the best scenario; but if I understand correctly, this is incompatible with the current installation technique we use for Makefile.config, which is just a copy whose output is expected to be greppable (so in particular that file could not include I think that the best route of action is to close the current and go with your version in #9332; if I understand your point correctly, it will make me happy (assuming correctly that I may hack on Makefile.common in the future, but much less on Makefile.build_config), and it also makes you happy for other reasons. Besides, I'm happy to leave the configure stuff to the configure experts! Closing. |
Makefile.commonwas generated by./configurejust to get the value of theINSTALLvariable. This PR moves theINSTALLvariable toMakefile.config, and stops configure-generatingMakefile.common. I think it is nicer and easier if onlyMakefile.configis generated by./configure.(cc @dra27 @shindere)
For me configure-generated files are a constant source of interface issues during my development: I go update the generated one, it doesn't show up in my
git status, then I need to realize what I did wrong and figure out how to fix it. It's a small pain but it comes up often, and these add up to a real inconvenience. This is my reason for getting rid ofMakefile.common.in.To understand how
configuresets INSTALL, I went to see the autoconf documentation on AC_PROG_INSTALL and I realized that it also defines variable INSTALL_DATA and INSTALL_PROGRAM, making the exact same distinction we introduced in #1680. I thus changed our ownINSTALL_DATAandINSTALL_PROGvariables to use the autoconf definitions, instead of redoing it on our own.Finally: the change touches a lot of file because having a generated Makefile.common induces a surprising amount of bureaucratic ceremony (in particular, @dra27 was careful to use
-includeinstead ofincludeon this file so that our Makefiles work before configure has run, and I decided to be brave and undo all of that). I think that this invasiveness is not a downside of the present PR; on the contrary, it highlights that each configure-generated file is a pain to deal with, so it supports the idea of the PR.