Various build-system fixes and enhancements#12108
Merged
shindere merged 9 commits intoocaml:trunkfrom Mar 18, 2023
Merged
Conversation
This commit contains two changes to the root Makefile: 1. Removal of the following line: $(DEPDIR)/runtime/%.bpic.$(D): OC_CFLAGS += $(SHAREDLIB_CFLAGS) This line is useless because it makes no sense to alter any variable in the CFLAGS category for the computation of dependencies, since only the C preprocessor is involved in this stage and it does not take C flags into account anyway, only C preprocessor flags. 2. When computing the dependencies for $(DEPDIR)/runtime/%.npic.$(D), one should not refer to $(SHAREDLIB_CFLAGS), for a similar reason. It has been verified that SHAREDLIB_CFLAGS is either empty, or contains just -fPIC which is indeed not necessary for computing dependencies (it is indeed a C flag rather than a C preprocessor flag).
In this target-specific definition, C and C preprocessor flags were mixed. This commit distinguishes one form the other.
Given the convention that the OC_* build varialbes are reserved for the build system, it seems better to make sure all of them are defined in the private Makefile.build_config file, rather than in Makefile.config which gets installed and thus becomes public. This commit moves the definitions of OC_CFLAGS, OC_CPPFLAGS, OC_LDFLAGS, OC_DLL_LDFLAGS and OC_EXE_LDFLAGS from Makefile.config.in to Makefile.build_config.in. It also moves the defintion of MKEXE_VIA_CC, since this variable relies on private build varables and does not seem relevant or useful outside of the context of the build of the compiler itself.
The renamings done in this commit are: OC_COMMON_CFLAGS -> OC_COMMON_COMPFLAGS OC_COMMON_LDFLAGS -> OC_COMMON_LINKFLAGS OC_BYTECODE_LDFLAGS -> OC_BYTECODE_LINKFLAGS OC_NATIVE_CFLAGS -> OC_NATIVE_COMPFLAGS OC_NATIVE_LDFLAGS -> OC_NATIVE_LINKFLAGS
This means moving its definition from Makefile.common to Makefile.build_config.in
This is to let configure specify flags that will be used when compiling C files to be linked with native code. The variable is not used in the build system yet, just defined.
993723a to
8e3c87d
Compare
5 tasks
When compiled for linking with native code, C files use both the common preprocessor flags and the native-specific cppflags. The same should happen for assembly files and this commit makes sure this is the case.
8e3c87d to
8f26653
Compare
MisterDA
approved these changes
Mar 17, 2023
Contributor
MisterDA
left a comment
There was a problem hiding this comment.
We've reviewed the PR offline, and all the changes make sense to me. Thanks Sébastien!
Contributor
Author
|
Antonin Décimo (2023/03/17 08:44 -0700):
@MisterDA approved this pull request.
We've reviewed the PR offline, and all the changes make sense to me.
Thanks Sébastien!
Thanks to you, Antonin. To complete what Antonin explained, let me said
that we really went through the PR on a commit by commit basis. Antonin
asked relevant questions for each commit andhe really tried hard to
understand themore general context and, I think, he definitely succeded
to do so. He nnoticed alittle mistakein one commit: some compiler flags
were passed twice. It ddin'thurt but was useless andhas been fixed as
part of our in-person review process.
I am grateful to Antonin for his patience andhis determination. I do
think the review he did is a totally genuine one, not shallow at all.
|
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The original purpose of this PR was to provide a configurable build variable
to provde C-compiler flags to be used specifically when compiling C object files for
linking with native code.
This is going to be useful for TSan, the threadsanitizer, but has been
designed to be general and suableby other features.
Finally this PR consists in a series of commits that enhance and fix the
build system in various ways.
It's best reviewed commit by commit and the individual commit messages
hopefully explain each change.
Also, each commit is conciseandshould be rather self-explanatory and easy to
review.