Merge utils/Makefile into the root Makefile#11268
Conversation
|
Is it okay to stop generating These files look not so useful since the corresponding implementations |
4423681 to
8393bb7
Compare
These files are there so that if you add, say, Otherwise you only find this out when |
This reads like a design aspect that could be documented by a comment somewhere in the configuration / build system. (Maybe the PR could include such a comment?) |
Guilty as charged! It could/should indeed be documented with the definition of |
|
David Allsopp (2022/05/24 02:59 -0700):
> Is it okay to stop generating `utils/config_boot.mli` and `utils/config_main.mli` as the PR does?
>
> These files look not so useful since the corresponding implementations won't actually be compiled as such but rather copied to `config.ml` which will then be compiled and checked against `config.mli`.
These files are there so that if you add, say, `Config.my_amazing_new_feature_flag` to `utils/config.mli` and `utils/config.mlp`, then during the build you get:
```
File "/home/dra/ocaml/utils/config_boot.ml", line 1:
Error: The implementation utils/config_boot.ml
does not match the interface utils/config_boot.cmi:
The value `my_amazing_new_feature_flag' is required but not provided
File "/home/dra/ocaml/utils/config_boot.mli", line 262, characters 0-17:
Expected declaration
```
Otherwise you only find this out when `make bootstrap` is next run
(hopefully in CI, of course, but nonetheless further down the line).
I'll try to do it with the recently added `-cmi-file` compiler option
which should avoid the need to cpy files around.
|
740d98f to
1e83077
Compare
|
Gabriel Scherer (2022/05/24 03:09 -0700):
> These files are there so that [..]
This reads like a design aspect that could be documented by a comment
somewhere in the configuration / build system. (Maybe the PR could
include such a comment?)
Let's first see whether we can get rid of it or not and then, yes,
definitely, if it stays in place it will be worth documenting it. but I
hope we can get rid of it in an elegant manner.
|
1e83077 to
5cc0e82
Compare
|
Although there was no conflict with trunk it felt a good idea to rebase. |
5cc0e82 to
20b315a
Compare
|
Just rebased on latest trunk to resolve merge conflicts |
dra27
left a comment
There was a problem hiding this comment.
It is really great to see the back of those sed expressions, and sorry it's taken me a while to review this. I've checked the various files generated commit-by-commit to try to try to catch the effect of all the moving parts.
A few of the comments are suggestions for clarifications or tweaks, but they're mostly optional: in particular, I think possibly a little too much has been removed from ocamltest/Makefile into configure.ac and there's a slightly muddying of the proverbial waters because there's some extant win32unix/unix code in ocamltest which you might want to excise at the same time.
Makefile.config.in
Outdated
|
|
||
| ### How to tell the C compiler to output an object file | ||
| OUTPUTOBJ=@outputobj@ | ||
| OUTPUTOBJ=@outputobj@ $(EMPTY) |
There was a problem hiding this comment.
The space before $(EMPTY) here needs to go.
configure.ac
Outdated
| [PACKLD="link -lib -nologo $machine -out:"], | ||
| [PACKLD="$DIRECT_LD -r$PACKLD_FLAGS -o \$(EMPTY)"])], | ||
| [PACKLD="$PARTIALLD -o \$(EMPTY)"]) | ||
| [pacld_ocaml="link -lib -nologo $machine -out:" |
There was a problem hiding this comment.
Typo: this should be packld_ocaml (missing k) - although suggestion earlier that this separate variable isn't needed.
configure.ac
Outdated
| AC_SUBST([flexdll_chain]) | ||
| AC_SUBST([mkdll_ldflags_exp]) | ||
| AC_SUBST([PACKLD]) | ||
| AC_SUBST([packld_ocaml]) |
There was a problem hiding this comment.
I don't think this variable is needed - $(EMPTY) is used because some linkers must have a space after -o. There's no harm for the Microsoft case having configure set PACKLD="link -lib -nologo $machine -out:" (no space at the end) and then have in Makefile.config.in PACKLD=@PACKLD@$(EMPTY)? @PACKLD@ can then be used directly in utils/config_fragment.ml.in
configure.ac
Outdated
|
|
||
| ## Determine which flags to use for the C compiler | ||
|
|
||
| outputobj='-o ' |
There was a problem hiding this comment.
I find this a little inconsistent: in the AS_CASE below, we very clearly set all outputobj, warn_error_flag, and cc_warnings for various different configurations.
If the default for outputobj goes here shouldn't the defaults for warn_error_flag and cc_warnings go here too?
configure.ac
Outdated
|
|
||
| # Absolute path to the toplevel source directory | ||
|
|
||
| # A sentinelle character (X) is added ad the end of the directory name |
There was a problem hiding this comment.
Two typos: at the end of the directory name and sentinel, rather than sentinelle. I would be more explicit on the exact reason for the sentinel: it's there because $(..) strips trailing \n characters (you could reference Eric's marvellous reply in https://lists.gnu.org/archive/html/autoconf/2019-07/msg00002.html !)
| @@ -1,127 +0,0 @@ | |||
| #************************************************************************** | |||
There was a problem hiding this comment.
The SUBST and SUBST_STRING macros in Makefile.common can also be deleted along with this file!
Makefile.config.in
Outdated
|
|
||
| ### How to tell the C compiler to output an object file | ||
| OUTPUTOBJ=@outputobj@ | ||
| OUTPUTOBJ=@outputobj@ $(EMPTY) |
There was a problem hiding this comment.
The space before $(EMPTY) shouldn't be here (same in OUTPUTEXE on L192)
Makefile.config.in
Outdated
|
|
||
| ### How to tell the C compiler to output an object file | ||
| OUTPUTOBJ=@outputobj@ | ||
| OUTPUTOBJ=@outputobj@ $(EMPTY) |
There was a problem hiding this comment.
The space before $(EMPTY) shouldn't be here (same in OUTPUTEXE on L192)
| let libunix = @ocamltest_libunix@ | ||
|
|
||
| let systhreads = %%systhreads%% | ||
| let systhreads = @lib_systhreads@ |
There was a problem hiding this comment.
@systhread_support@ could be used, and the extra variable lib_systhreads eliminated.
| mkdll_ldflags="${mkdll_ldflags} ${mkexe_ldflags_prefix}${flag}" | ||
| done]) | ||
| done | ||
| mkdll_ldflags_exp="$mkdll_ldflags"]) |
There was a problem hiding this comment.
I think a mkexe_ldflags_exp is needed here, too (see later comment in utils/config_fragment.ml.in. It would want the oc_ldflags part below on L2231 as well but then not the oc_dll_ldflags bit on L2237
06799f4 to
384389a
Compare
bceefe0 to
e8b2315
Compare
369e685 to
d776671
Compare
Add the $(EMPTY) space-preserving suffix to OUTPUTOBJ, OUTPUTEXE and PACKLD in Makefile.config.in rather than in the configure script. This makes it possible to substitute these variables in OCaml files during the configure stage. This commit also makes the mkdll_ldflags_exp variable available to the build system and improves its computation to ensure it does not contain superfluous spaces. Finally, this commit instanciates CFLAGS and CPPFLAGS during the configure stage, so that they can be passed to the OCaml configuraiton module. It becomes thus impossible to override CFLAGS and CPPFLAGS at build time.
This will be used to generate the OCaml configuration module as part of the configure stage, rather than generating it during the build.
This is the fragment of the configure module which is presently generated at build time from a template file but will ultimately be generated at configure time. Also make .gitignore more specific and explicit about what needs to be ignored.
This commit also improves the way mkdll_ldflags_exp is computed so that it contains no spurious spaces.
d776671 to
35c93d2
Compare
|
I just rebased this on latest trunk, after the merge of #11420. Rebasing required to resolve three merge conflicts in configure. That looks as another argument not to have configure versionned, except for This should be taken into account in #11343 |
|
Note: One simple way to resolve a conflict on a generated file is to re-generate it. |
|
Gabriel Scherer (2022/09/07 02:37 -0700):
Note: One simple way to resolve a conflict on a generated file is to
re-generate it.
Yes, of course, and it's what I did, three times, but it's both manual
and useless.
|
dra27
left a comment
There was a problem hiding this comment.
Precheck seems happy. With some slight adaptations, I persuaded this PR to produce utils/config.generated.ml files for the presently disabled Cygwin/MSVC ports and compared these with utils.config.ml from 4.14 (along with mingw).
What this revealed is some breakage from #10413 which we didn't pick up on (it affects mingw-w64 somewhat innocuously, but the faults break MSVC and Cygwin).
However, this PR doesn't make those problems on trunk/5.0 any worse, so I propose as a penance to open a PR to fix those to be merged after this one.
Nice work, @shindere! I'll aim not to shed a tear for the loss of a 50 line sed command :)
|
David Allsopp (2022/09/09 07:28 -0700):
@dra27 approved this pull request.
[Precheck seems happy](https://ci.inria.fr/ocaml/job/precheck/761/).
With some slight adaptations, I persuaded this PR to produce
`utils/config.generated.ml` files for the presently disabled
Cygwin/MSVC ports and compared these with `utils.config.ml` from 4.14
(along with mingw).
I am grateful for all the time and care you invested in reviewing this
ugly PR. Many, many thanks, @dra27. In my opinion, for this PR reviewing
was at least as hard as doingit and @dra27 has been a great, meticuluous
and supportive reviewer.
What this revealed is some breakage from #10413 which we didn't pick
up on (it affects mingw-w64 somewhat innocuously, but the faults break
MSVC and Cygwin).
Oops, so sorry. The present PR and #10413 are really tough bits of
code...
However, this PR doesn't make those problems on trunk/5.0 any worse,
so I propose as a penance to open a PR to fix those to be merged after
this one.
Many thanks again. I'll make sure to review the PR you'll open.
Nice work, @shindere! I'll aim not to shed a tear for the loss of a 50
line `sed` command :)
:-)
This also means we don't have any `.plp` files in our codebase, which
feels great.
|
|
David Allsopp (2022/09/09 07:33 -0700):
Merged #11268 into trunk.
Hurra! :)
|
|
This PR broke bootstrapping: there are some dependencies on |
|
Oops - it's not to do with the |
|
David Allsopp (2022/05/24 02:59 -0700):
> Is it okay to stop generating `utils/config_boot.mli` and `utils/config_main.mli` as the PR does?
>
> These files look not so useful since the corresponding implementations won't actually be compiled as such but rather copied to `config.ml` which will then be compiled and checked against `config.mli`.
These files are there so that if you add, say, `Config.my_amazing_new_feature_flag` to `utils/config.mli` and `utils/config.mlp`, then during the build you get:
```
File "/home/dra/ocaml/utils/config_boot.ml", line 1:
Error: The implementation utils/config_boot.ml
does not match the interface utils/config_boot.cmi:
The value `my_amazing_new_feature_flag' is required but not provided
File "/home/dra/ocaml/utils/config_boot.mli", line 262, characters 0-17:
Expected declaration
```
Otherwise you only find this out when `make bootstrap` is next run
(hopefully in CI, of course, but nonetheless further down the line).
I restored the generation of those files. Still, I am wondering whether
the same result couldn't be achieved in another, more straightforward
way. For instance could `config_boot.mli` and `ocnfig_main.mli` be
defined to somehow incllude the signature in `config.mli`? Perhaps we
could introduce, in `config.mli`, an `OCaml_Config` module type that
would contain all the specification of the configuration module and that
could then be included in `config.mli` and then from `config_boot.mli`
and `config_main.mli`, or something similar?
I am also wondering about the following dependency from `compilerlibs/Makefile.compilerlibs`:
```
compilerlibs/ocamlcommon.cma: $(COMMON_CMI) $(ALL_CONFIG_CMO) $(COMMON)
```
I understand its purpose, which you clearly explained above, but it
raises two questions to me.
First, this happens only on the bytecode side. IF we one day manage to
compile the native code directly, we will miss this dependency.
Second, since the dependency is not a real one (e.g.,
`compilerlibs/ocamlcommon.cma` does not really depend on
`config_boot.cmo` when we are not bootstrapping), it feels to me it
would be clearer and cleaner to define a proper target to build compiler
libs and that would build all the appropriate objects, rather than to
use make's dependencies to obtain side effects.
If desired I can include modifications in this PR, or leave that for
later, as is preferred.
|
The main thing this PR does is to modify the configure and build systems
so that
ocamltest/ocamltest_config.mlandutils/config.ml(renamed toutils/config_fragment.mlby this PR) are generated as part of theconfigure stager, rather than during the build.
This PR consists in several commits which are best reviewed one after the
other.
Two questions regarding the configure module:
Wouldn't it be a good moment to remove
bytecode_c_compilerand the like, or do those need to be deprecated first?The code that computes
mkexe,mkdllandmkmaindllis executed at runtime. Is that a feature, i.e. a behaviour that needs to be preserved as it it, or could we save a test and a few computations and generate the correct code at configure time?