-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -97,7 +97,7 @@ O=@OBJEXT@ | |||
EXT_OBJ=.@OBJEXT@ | |||
|
|||
### How to tell the C compiler to output an object file | |||
OUTPUTOBJ=@outputobj@ | |||
OUTPUTOBJ=@outputobj@ $(EMPTY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: this should be packld_ocaml
(missing k) - although suggestion earlier that this separate variable isn't needed.
configure.ac
Outdated
AC_SUBST([PACKLD]) | ||
AC_SUBST([packld_ocaml]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -591,19 +652,18 @@ AS_IF( | |||
|
|||
## Determine which flags to use for the C compiler | |||
|
|||
outputobj='-o ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SUBST
and SUBST_STRING
macros in Makefile.common
can also be deleted along with this file!
Makefile.config.in
Outdated
@@ -97,7 +97,7 @@ O=@OBJEXT@ | |||
EXT_OBJ=.@OBJEXT@ | |||
|
|||
### How to tell the C compiler to output an object file | |||
OUTPUTOBJ=@outputobj@ | |||
OUTPUTOBJ=@outputobj@ $(EMPTY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The space before $(EMPTY)
shouldn't be here (same in OUTPUTEXE
on L192)
Makefile.config.in
Outdated
@@ -97,7 +97,7 @@ O=@OBJEXT@ | |||
EXT_OBJ=.@OBJEXT@ | |||
|
|||
### How to tell the C compiler to output an object file | |||
OUTPUTOBJ=@outputobj@ | |||
OUTPUTOBJ=@outputobj@ $(EMPTY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The space before $(EMPTY)
shouldn't be here (same in OUTPUTEXE
on L192)
|
||
let systhreads = %%systhreads%% | ||
let systhreads = @lib_systhreads@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@systhread_support@
could be used, and the extra variable lib_systhreads
eliminated.
@@ -2138,18 +2221,20 @@ AC_CONFIG_COMMANDS_PRE([ | |||
AS_IF([test -n "${LDFLAGS}"], | |||
[for flag in ${LDFLAGS}; do | |||
mkdll_ldflags="${mkdll_ldflags} ${mkexe_ldflags_prefix}${flag}" | |||
done]) | |||
done | |||
mkdll_ldflags_exp="$mkdll_ldflags"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.ml
andutils/config.ml
(renamed toutils/config_fragment.ml
by 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_compiler
and the like, or do those need to be deprecated first?The code that computes
mkexe
,mkdll
andmkmaindll
is 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?