Get rid of the stdlib/Compflags script#8601
Conversation
gasche
left a comment
There was a problem hiding this comment.
It would be nice to have some explanation, in comments, of how the code work, so that people that are not experts in horrible GNU Make tricks have a chance of being able to locate this part of the logic and modify it if necessary. I feel like the shell script was approachable, but the new code less so.
(I expect the gains to be related to faster compilation due to reduced process forking, with possibly larger effects on Windows?)
stdlib/Makefile
Outdated
|
|
||
| stdlib__scanf.cmx: COMPFLAGS += -inline 9 | ||
|
|
||
| %Labels.cmo: private COMPFLAGS += -nolabels -no-alias-deps |
There was a problem hiding this comment.
what does private mean here and below?
There was a problem hiding this comment.
There was a problem hiding this comment.
I think it’s to replicate the behaviour of the case in the script - if you add a specific rule for one of these files, then this rule would no longer apply. I’m not sure why it’s on the next one, though, @shindere?
There was a problem hiding this comment.
Thanks for the pointer. Do you understand why it is used for %Labels.cmo but not for %Labels.cmx, and with the double-target rule below but not the double-target rule above? (Maybe we could consistently use private in all cases of this hidden pattern-matching for simplicity?)
|
This is very nice! One thought, which might aid future debugging - perhaps the switches could be added to |
|
Gabriel Scherer (2019/04/09 15:01 +0000):
It would be nice to have some explanation, in comments, of how the
code work,
well we started to use similar things in `runtime/Makefile` without
explaining them so I'm not sure why it should happen here rather than
there...
so that people that are not experts in horrible GNU Make
tricks have a chance of being able to locate this part of the logic
and modify it if necessary.
Well it's not that horrible really, IMO.
I feel like the shell script was
approachable, but the new code less so.
I'd say it's more a matter of habbits than something else.
(I expect the gains to be related to faster compilation due to reduced
process forking, with possibly larger effects on Windows?)
I think it has more to do with readability, actually.
At the same time, in my opinion this solution is still not ideal. In my
opinion, the best placefor these flags (or equivalent directives) would
be in the source files themselves.
+stdlib__buffer.cmx: COMPFLAGS += -inline 3
+
+stdlib__buffer.cmo: COMPFLAGS += -w A
+
+camlinternalFormat.cmo: COMPFLAGS += -w Ae
+
+camlinternalFormatBasics.cmo camlinternalFormatBasics.cmx: \
+ COMPFLAGS += -nopervasives
+
+stdlib__printf.cmo stdlib__format.cmo \
+stdlib__scanf.cmo: COMPFLAGS += -w Ae
+
+stdlib__scanf.cmx: COMPFLAGS += -inline 9
+
+%Labels.cmo: private COMPFLAGS += -nolabels -no-alias-deps
what does `private` mean here and below?
I guess @dra27 has addressed this.
Typically, the that, when a variable is made target-specific, then it
will be "inherited" by the target's prerequisites. Here for instance, a
variable defined for `.cmo` targets would be inherited by `.cmi` ones,
which is not always wanted. This is what "private" is used for.
|
|
David Allsopp (2019/04/09 15:09 +0000):
I think it’s to replicate the behaviour of the `case` in the script -
if you add a specific rule for one of these files, then this rule
would no longer apply. I’m not sure why it’s on the next one, though,
@shindere?
Sorry @Êra27: I am reading you in an e-mail and I am not understanding
your question. Would you mind explaining a bit more what you would like
me to clarify?
|
|
Gabriel Scherer (2019/04/09 08:10 -0700):
Thanks for the pointer. Do you understand why it is used for
`%Labels.cmo` but not for `%Labels.cmx`, and with the double-target
rule below but not the double-target rule above? (Maybe we could
consistently use `private` in all cases of this hidden
pattern-matching for simplicity?)
I dind't try to be consistent here. I added the private directives where
they were required to obtain, as a result, an output of `make world.opt`
which is completely equivalent to what we had before. I wouldn't mind
adding more private directives if there is a reason to do so, but I
think the commit is already correct in its current state.
|
|
David Allsopp (2019/04/09 08:11 -0700):
This is very nice! One thought, which might aid future debugging -
perhaps the switches could be added to `ADDCOMPFLAGS` (or another name
of that already exists) and `COMPFLAGS` remain untouched with
`ADDCOMPFLAGS` being empty by default
Well, it could be done, yes. But the code in its current state does,
IMO, kind of a minimal change so I'd rather leave it as it is and do
further changes later.
|
|
@shindere - the change would alter every deletion of |
|
I would really like to see this code (1) documented and (2) made consistent. If I can't understand what it does and how further changes to it should be written, it's reasonable to assume that many other contributors will be in the same situation. I am not in favor of a code that makes the build system harder to understand and evolve if the only benefit is "readability" as perceived by the three people in the project who know too much Make for their own good. |
|
David Allsopp (2019/04/09 12:42 -0700):
@shindere - the change would alter every deletion of `$(shell
./Compflags $@)` to `$(ADDCOMPFLAGS)` and otherwise only affect the
added lines, adding one `ADDCOMPFLAGS =` line? It would also guard
against anything external overriding of `COMPFLAGS` (I'm haven't
checked thoroughly whether this is possible)
Okay the change would not be much bigger, but what's the point of not
using COMPFLAGS, actually? It's not really a standard variable, so I am
not really getting your point.
I think you also mentionned that using another variable would make
debugging easier but I have to say I didn't understand how?
|
|
Gabriel Scherer (2019/04/09 14:01 -0700):
I would really like to see this code (1) documented
So, hy didn't you request this in `runtime/Makefile` which IMO is more
complicated and (private put apart) also uses target-specific variables
and even complex macro definitions?
If these target-specific variables are used in several places in the
build system, are you saying this should be documented in every place it
is used? Or is it just the "provate" you would like to see documented?
and (2) made consistent.
What do you mean by consistent?
|
ed028b7 to
baa785e
Compare
This was an oversight on my part, please also document
I would appreciate if the story for where to put |
This script was used to provide module-specific compiler flags. Now that we use GNU make, these flags can be handled by make itslef.
baa785e to
40c88ee
Compare
|
Gabriel Scherer (2019/04/09 23:42 -0700):
> So, hy didn't you request this in `runtime/Makefile` which IMO is more complicated and (private put apart) also uses target-specific variables and even complex macro definitions?
This was an oversight on my part, please also document
`runtime/Makefile`.
Well that PR had been reviewed and nobody complained about the lack of
documentation. I can still consider documenting it in the future, but in
my opinion this wouldn't fit in the present PR.
> What do you mean by consistent?
I would appreciate if the story for where to put `private` was simpler
than "this is what @shindere experimentally figured were the minimal
additions" -- how can another user go extend that if they want to add
a new case? If I understand what `private` is doing here (but I
probably don't), it should be correct to put `private` in *all* the
definitions; in that case I would be much in favor of doing that, as
it gives a clear rationale that is easy to understand and follow.
Please see the updated version. It also addresses @dra27's suggestion to
have the module-specific flags in a dedicated variable, MODCOMPFLAGS.
|
That's fine with me.
Thanks! The new version looks nice indeed, and in particular I understand the explanation for the usage of |
|
Gabriel Scherer (2019/04/10 01:42 -0700):
gasche approved this pull request.
Thanks!!
|
|
Just pushed b29e897 to trunk to fix a typo. But, unfortunately, that's not the end of the story: it seems "private" Working on an alternative version that does not need provate. Will submit a new PR for that later today. |
|
As a conclusion of the discussion that took place on #8608, this PR has |
this PR implements a suggestion by @xavierleroy.
The
stdlib/Compflagsscript was used to provide module-specificcompiler flags. Now that we use GNU make, these flags can be
handled by make itslef.
It has been verified that the outputs of
make world.optbefore andafter this commit are equivalent. They are not identical, but their only
differences are aobut spacing and the fact that, before the commit,
the
AWKvariable was expanded by the subshell invoked bymake,whereas after the commit, the variable is expanded by make itself.