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

Allow passing in CFLAGS(_EXTRA) #483

Closed
wants to merge 1 commit into from

Conversation

plajjan
Copy link
Contributor

@plajjan plajjan commented Oct 19, 2022

By using ?= we only set the CFLAGS variable in case it doesn't already have a value. This means it is now possible for someone to control the way bdwgc is compiled by feeding in the CFLAGS variable whereas previously one had to modify the Makefile.direct file. Further, since we consider the CFLAGS a reasonable default, the primary means of injecting control flags is by setting CFLAGS_EXTRA, where we now also allow passing in a value and the stuff in Makefile.direct is appended to that input.

This is very useful for using bdwgc from another repo, like you can just mirror bdwgc and then compile by feeding in all the compilation options as environment variables rather than having to first write them to the Makefile.direct file, which can be rather tricky to do programmatically. The normal mode of working is probably to CFLAGS_EXTRA, since that gets merged with the CFLAGS in Makefile.direct, but it also allows to directly set CFLAGS, thus overriding the default value of CFLAGS for total control.

By using ?= we only set the CFLAGS variable in case it doesn't already
have a value. This means it is now possible for someone to control the
way bdwgc is compiled by feeding in the CFLAGS variable whereas
previously one had to modify the Makefile.direct file. Further, since we
consider the CFLAGS a reasonable default, the primary means of injecting
control flags is by setting CFLAGS_EXTRA, where we now also allow
passing in a value and the stuff in Makefile.direct is appended to that
input.
@ivmai
Copy link
Owner

ivmai commented Oct 19, 2022

Okay

@ivmai
Copy link
Owner

ivmai commented Oct 20, 2022

I've modified your commit a bit using ?= for CFLAGS_EXTRA, as well as for CXXFLAGS and ABI_FLAGS. Will push it today.

ivmai pushed a commit that referenced this pull request Oct 21, 2022
Issue #483 (bdwgc).

By using ?= we only set the CFLAGS variable in case it doesn't already
have a value.  This means it is now possible for someone to control
the way bdwgc is compiled by feeding in the CFLAGS variable whereas
previously one had to modify the Makefile.direct file.  Further, since
we consider the CFLAGS a reasonable default, the primary means of
injecting control flags is by setting CFLAGS_EXTRA, where we now also
allow passing in a value and the stuff in Makefile.direct is appended
to that input.  Same for [AS_]ABI_FLAG and CXXFLAGS variables.

* Makefile.direct (ABI_FLAG, AS_ABI_FLAG, CFLAGS_EXTRA, CFLAGS,
CXXFLAGS): Assign value using operator "?=" instead of "=" one.
@ivmai
Copy link
Owner

ivmai commented Oct 21, 2022

Merged as f7cfa9d

@ivmai ivmai closed this Oct 21, 2022
@plajjan
Copy link
Contributor Author

plajjan commented Oct 21, 2022

Cool, thanks! =)

@plajjan plajjan deleted the enable-cflags-input branch October 21, 2022 09:47
@ivmai
Copy link
Owner

ivmai commented Oct 21, 2022

@plajjan
Copy link
Contributor Author

plajjan commented Oct 21, 2022

Without looking at any core or errors, I'm guessing it is setting CFLAGS which are now used?

Guess test environment need to set whatever should be tested...

@ivmai
Copy link
Owner

ivmai commented Oct 21, 2022

Strange but CFGLAGS are empty when passed to the compiler (cc).

@ivmai
Copy link
Owner

ivmai commented Oct 21, 2022

Last good build (bbe9092) before the change: https://app.travis-ci.com/github/ivmai/bdwgc/jobs/584941687

@ivmai
Copy link
Owner

ivmai commented Oct 21, 2022

Before (success):

cc  -O -I./include -I./libatomic_ops/src  -DALL_INTERIOR_POINTERS -DENABLE_DISCLAIM -DGC_ATOMIC_UNCOLLECTABLE  -DGC_GCJ_SUPPORT -DJAVA_FINALIZATION -DNO_EXECUTE_PERMISSION  -DUSE_MMAP -DUSE_MUNMAP   -c -I. ./cord/cordbscs.c

After (fail):

cc  -O2 -pipe -c -I. ./cord/cordbscs.c

@ivmai
Copy link
Owner

ivmai commented Oct 21, 2022

It looks like as if CFLAGS="-O2 -pipe" is passed to make, but I don't understand why.

@ivmai
Copy link
Owner

ivmai commented Oct 21, 2022

It looks like that the system defines default CFLAGS (probably in "/etc/portage/make.conf" as said in https://wiki.gentoo.org/wiki/GCC_optimization#What_are_CFLAGS_and_CXXFLAGS.3F).
Previously C[XX]FLAGS were overwritten in Makefile.direct.

@ivmai
Copy link
Owner

ivmai commented Oct 22, 2022

I'm working on the fix, along with some more refactoring

ivmai added a commit that referenced this pull request Oct 22, 2022
(fix of commit f7cfa9d)

Issue #483 (bdwgc).

Some hosts (e.g. FreeBSD ones) predefine CFLAGS variable (to e.g. -O2),
thus the options required to build libgc (e.g. -I) should be appended
to CFLAGS.

If there is a need to alter the default macros added to CFLAGS in
Makefile.direct, then the client should provide the full list of -D
options in CFLAGS_DEFAULT_MACROS variable.

* Makefile.direct (CFLAGS_FOR_PIC): Set to empty (if unset).
* Makefile.direct (CFLAGS_DEFAULT_MACROS): New variable (move all -D
options from CFLAGS) which could be customized by client.
* Makefile.direct (CFLAGS): Add comment; split assignment into two
steps (assign "-O" value first using "?=" operator, then append the
remaining options using "+=" operator); use CFLAGS_DEFAULT_MACROS.
@ivmai
Copy link
Owner

ivmai commented Oct 22, 2022

Should be fixed now. The required -I and other options are appended to CFLAGS,

From now on, if you need some custom -D passed to Makefile.direct, it is better to set CFLAGS_DEFAULT_MACROS.

ivmai pushed a commit that referenced this pull request Nov 17, 2022
Issue #483 (bdwgc).

* Makefile.direct (CC, CXX): Assign value using operator "?=" instead
of "=" one.
ivmai added a commit that referenced this pull request Jan 1, 2023
(fix of commit f7cfa9d)

Issue #483 (bdwgc).

AIX Make seems not to recognize "?=" operator properly.

The proposed workaround to duplicate all flags of CFLAGS to CXXFLAGS
(appended to the user-supplied CXXFLAGS if any).

* Makefile.direct (CXXFLAGS): Set to empty value (using "?=");
append all flags of CFLAGS (i.e., expand CFLAGS manually).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants