Skip to content
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

Build system enhancements #1840

Merged
merged 10 commits into from Jun 20, 2018

Conversation

Projects
None yet
4 participants
@shindere
Copy link
Contributor

commented Jun 15, 2018

The initial intent behind this PR was to start preparing the transition to
autoconf. More specifically, in autoconf-based packages, a few build
variables (e.g. CFLAGS, CPPFLAGS, LDFLAGS) are expected to be
available to the "user" (the person
compiling the package).

So the initial aim of this PR was to renaim the above mentionned build
variables so that they become private to the build system and so that
they can later be re-introduced but without any value so that users can
freely use them without
unintentionnally breaking the build system.

So this PR does that, plus a few additiolnal things in the build system of
the runtime:

  • For the debugging, intstrumentation, profiling and PIC related flags,
    clarify which are compiler and which are preprocessor flags

  • Factorise the rules to compile C files. This is to make it easier
    to re-introduce the user variables later by decreasing the number of
    places in the build system where they need to be mentionned

@adrien-n: you may be interested in reviewing this

@dra27
Copy link
Contributor

left a comment

Unless 4.08 is definitely going to have the configure script available for Windows, I'm dubious of changes which involve renaming variables in config/Makefile as it increases still further the headache for configuring Windows-builds (as demonstrated by the changes needed to the Inria CI scripts here!).

Sorry about the comment on the shed colour, but as it's its introduction: I found the OC_ prefix a little odd to read - can't we just have OCAML_?

For now, I wonder if this scheme might be a more compatible first step:

  • Don't rename SHAREDCCCOMPOPTS (but I do agree that SHAREDLIB_CFLAGS is a much better name, once that name is truly internal to the build system!)
  • Keep CFLAGS, CPPFLAGS and LDFLAGS in config/Makefile (and the 4 Windows Makefiles, therefore)
  • At the top of Makefiles which use CFLAGS, CPPFLAGS, and LDFLAGS, add OC_CFLAGS=$(CFLAGS) ... with the intention that those lines get removed when config/Makefile starts generating the "correct internal files.

A question on the CFLAGS stuff - I thought that the autoconf idea was that the user should be able to specify additional flags which they'd like here (or override them). So in the places where $(CFLAGS) is being changed to $(OC_FLAGS) shouldn't it be becoming $(CFLAGS) $(OC_FLAGS) - i.e. we start to move the flags we determined during configure, and which are recorded in config/Makefile to OC_FLAGS but we still allow the user to specify CFLAGS themselves? Note that this idea is orthogonal to my third suggestion above (adding OC_FLAGS=$(CFLAGS)) as that would duplicate the flags given in CFLAGS, obviously.

%.p.$(O): %.c
$(CC) -c $(PFLAGS) $(CPPFLAGS) $(OUTPUTOBJ)$@ $<
$(foreach object_type, $(object_types), \
$(eval $(call COMPILE_C_FILE,$(object_type))))

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

I know it's equivalent, but part of me would prefer the definition of the build rule (via eval) to be physically below the setting up of dependencies (in this case the env. variables)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 20, 2018

Member

I agree with @dra27: for readability it should be moved down.

sed -e 's/\.o/.p.$$(O)/' >> .depend
$(CC) -MM $(OC_CPPFLAGS) $(OC_DEBUG_CPPFLAGS) $^ | \
sed -e 's/\.o/.d.$$(O)/' >> .depend
$(CC) -MM $(OC_CPPFLAGS) $(OC_INSTR_CPPFLAGS) $^ | \

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

In all three of these, I think it may be relevant to have had PFLAGS, DFLAGS and IFLAGS first - doesn't GCC go with the first option encountered? In other words, switching it around I think causes CPPFLAGS always to win?

(I'm not convinced this matters in practice, I'm just wondering if the change is necessary)

@@ -42,7 +42,7 @@ endif
endif

ifdef BOOTSTRAPPING_FLEXLINK
CFLAGS += -DBOOTSTRAPPING_FLEXLINK
OC_CPPFLAGS += -DBOOTSTRAPPING_FLEXLINK

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

Oops, as the original author of that error 😊


%.i.$(O): OC_CPPFLAGS += $(OC_INSTR_CPPFLAGS)

%.pic.$(O): OC_CFLAGS += $(SHAREDLIB_CFLAGS)

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

It feels like this should be shared between asmrun and byterun, rather than duplicated? Given that we're getting into the realm of GNU make macros!

$(CC) -MM $(IFLAGS) $(OC_CPPFLAGS) *.c | sed -e 's/\.o/.i.$$(O)/'>>.$@
$(CC) -MM $(OC_CPPFLAGS) $(OC_INSTR_CPPFLAGS) *.c | \
sed -e 's/\.o/.i.$$(O)/' >> .$@
$(CC) -MM $(OC_CPPFLAGS) *.c | sed -e 's/\.o/.pic.$$(O)/'>>.$@

This comment has been minimized.

Copy link
@dra27

dra27 Jun 15, 2018

Contributor

Same comment about ordering as in asmrun/Makefile

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

@dra27: regarding your initial remark about the changes to
config/Makefile. Given really expect the migrationto autoconf will be
complete before the 4.08 freeze, I'd like to make the following proposal.

Assuming the state of config/Makefile is less important between the
releases, could we go with the PR as it is given that, in the unlikely case
where the migration to autoconf would not be completed, then I would do a
commit to restore config/Makefilein the state it was before?

Does that look as a reasonable compromise to you?

@shindere shindere force-pushed the shindere:build-system-enhancements branch from 888c825 to fbad127 Jun 18, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

Rebased on latest trunk and made sure this PR does not introduce any
check-typo violation, now that our trunk is clean ;-)

@shindere shindere force-pushed the shindere:build-system-enhancements branch from fbad127 to f795a42 Jun 18, 2018

@xavierleroy
Copy link
Contributor

left a comment

I'm globally OK with the renaming of make variables. I'd like to be sure all the variables are documented somewhere (name and intended meaning). There is some effort in this direction in config/Makefile-templ but it's probably incomplete and outdated.

define COMPILE_C_FILE
$(1): %.c
$$(CC) -c $$(OC_CFLAGS) $$(OC_CPPFLAGS) $$(OUTPUTOBJ)$$@ $$<
endef

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jun 18, 2018

Contributor

I find this GNU make macro wizardry very hard to read. The result of the macro expansion (i.e. 5 implicit rules) would be more readable, not to mention more compact...

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

@shindere shindere force-pushed the shindere:build-system-enhancements branch 3 times, most recently from 8a041aa to b1d93a3 Jun 18, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

Can the doc wait until this is in place?

I'd rather not wait, for two reasons:

  • documentation makes reviewing easier
  • you'll be really sad at the end of the project when you have to write all the docs at once
@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

@shindere shindere force-pushed the shindere:build-system-enhancements branch 3 times, most recently from c0af612 to 78b0601 Jun 19, 2018

@damiendoligez
Copy link
Member

left a comment

Looks good except for a few things you should look into.

%.p.$(O): %.c
$(CC) -c $(PFLAGS) $(CPPFLAGS) $(OUTPUTOBJ)$@ $<
$(foreach object_type, $(object_types), \
$(eval $(call COMPILE_C_FILE,$(object_type))))

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 20, 2018

Member

I agree with @dra27: for readability it should be moved down.

>> .depend
$(CC) -MM $(IFLAGS) $(CPPFLAGS) $^ | sed -e 's/\.o/.i.$$(O)/' \
>> .depend
$(CC) -MM $(OC_CFLAGS) $(OC_CPPFLAGS) $^ | \

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 20, 2018

Member

Didn't you forget to remove $(OC_CFLAGS)?

COMPFLAGS=-I $(OTOPDIR)/otherlibs/str -I $(OTOPDIR)/otherlibs/unix
include $(TOPDIR)/Makefile.tools
uTOPDIR=../.. COMPFLAGS=-I $(OTOPDIR)/otherlibs/str -I
$(OTOPDIR)/otherlibs/unix include $(TOPDIR)/Makefile.tools

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jun 20, 2018

Member

Fat fingers? Your text editor has re-wrapped these three lines into two and you have an extraneous u at the beginning of the first one. This probably explains the Appveyor failure that we saw earlier today.

shindere added some commits Jun 20, 2018

Rename C compiler related build variables
This commit renames a few C compiler related build variables so that
they are reserved for the build system. They will then be re-introduced,
but this time as user varialbes whose value can be freely customized
when compiling the package, without risking to conflict with those
command-line flags that are required by the build system itself.

Here are the variables this commit renames:

- CFLAGS -> OC_CFLAGS
- CPPFLAGS -> OC_CPPFLAGS
- LDFLAGS -> OC_LDFLAGS

Note: before this commit the compilation of scheduler.c in
otherlibs/threads was relying on make's implicit rule to compile C files.

Since this commit stops using the standard variables for flags,
it is necessary to introduce an explicit rule to compile C files
and that makes use of the newly introduced variables.
byterun/Makefile: add preprocessor flags to the right varialbes
A few -D and -I flags were added to OC_CFLAGS while they should be added
to OC_CPPFLAGS

As a consequence, there is no need to pass OC_CFLAGS to the compiler
when computing dependencies for C files.

@shindere shindere force-pushed the shindere:build-system-enhancements branch from 78b0601 to d9b2d79 Jun 20, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2018

@shindere

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2018

@shindere shindere merged commit d9b2d79 into ocaml:trunk Jun 20, 2018

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shindere shindere deleted the shindere:build-system-enhancements branch Jun 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.