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.

Sign up for GitHub

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

Jump to bottom

build: actually set LDFLAGS/LIBS & stop overriding CFLAGS/LDFLAGS #5504

Merged
merged 4 commits into from
Dec 9, 2022
Merged

build: actually set LDFLAGS/LIBS & stop overriding CFLAGS/LDFLAGS #5504

merged 4 commits into from
Dec 9, 2022

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented Dec 4, 2022 •
edited
Loading

Commits:

$ git log --reverse --pretty='* %s' master..
* configure*: print CC and CFLAGS
* build: actually set LDFLAGS and LIBS in makefiles
* build: move library flags from EXTRA_LDFLAGS to LIBS
* makefiles: stop overriding CFLAGS/LDFLAGS

This is a follow-up to #5478 and #5488.

@kmk3
configure*: print CC and CFLAGS
55e3c58 
Currently, only EXTRA_CFLAGS and EXTRA_LDFLAGS are printed.

See also the variables defined on config.mk.in.
@kmk3
build: actually set LDFLAGS and LIBS in makefiles
671c3f2 
Both variables are used inside on src/prog.mk and src/so.mk, but they
are not currently defined in any makefile, so their values cannot be
substituted by ./configure.

This means that the variables can be set when running make (such as with
`make LDFLAGS=-Lfoo`), but changing them in configure.ac has no effect.
The same applies when trying to set them when running ./configure (such
as with `./configure LDFLAGS=-Lfoo`).
@kmk3
build: move library flags from EXTRA_LDFLAGS to LIBS
35332bd 
LIBS is the variable that Autoconf uses to put library flags.  From the
manual of GNU Autoconf (version 2.69):

>  -- Variable: LDFLAGS
>
>      [...]
>
>      This variable's contents should contain options like '-s' and '-L'
>      that affect only the behavior of the linker.  Please see the
>      explanation of 'CFLAGS' for what you can do if an option also
>      affects other phases of the compiler.
>
>      Don't use this variable to pass library names ('-l') to the linker;
>      use 'LIBS' instead.
>
>  -- Variable: LIBS
>
>      '-l' options to pass to the linker.  The default value is empty,
>      but some Autoconf macros may prepend extra libraries to this
>      variable if those libraries are found and provide necessary
>      functions, see *note Libraries::.  'configure' uses this variable
>      when linking programs to test for C, C++, Objective C, Objective
>      C++, Fortran, and Go features.
@kmk3
Copy link
Collaborator Author

kmk3 commented Dec 4, 2022 •
edited
Loading

Reproducing the last commit message here as these details might not be commonly
known:

makefiles: stop overriding CFLAGS/LDFLAGS

From the manual of GNU Automake (version 1.16.5)[1] [2]:

3.6 Variables reserved for the user

Some Makefile variables are reserved by the GNU Coding Standards for
the use of the "user"—the person building the package. For instance,
CFLAGS is one such variable.

Sometimes package developers are tempted to set user variables such
as CFLAGS because it appears to make their job easier. However, the
package itself should never set a user variable, particularly not to
include switches that are required for proper compilation of the
package. Since these variables are documented as being for the
package builder, that person rightfully expects to be able to override
any of these variables at build time.

To get around this problem, Automake introduces an
automake-specific shadow variable for each user flag variable.
(Shadow variables are not introduced for variables like CC, where
they would make no sense.) The shadow variable is named by prepending
AM_ to the user variable's name. For instance, the shadow variable
for YFLAGS is AM_YFLAGS. The package maintainer—that is, the
author(s) of the Makefile.am and configure.ac files—may adjust
these shadow variables however necessary.

Note Flag Variables Ordering::, for more discussion about these
variables and how they interact with per-target variables.

See also the description of CFLAGS in the GNU Autoconf manual[3].

Note: We do not use automake (save for aclocal) nor generally follow the
GNU Coding Standards, but the concept still applies. Also, the closest
analogous in the project to the AM_ prefix would currently likely be
EXTRA_.

[1] https://www.gnu.org/software/automake/manual/1.16.5/html_node/User-Variables.html
[2] https://www.gnu.org/software/automake/manual/1.16.5/html_node/Flag-Variables-Ordering.html
[3] https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Preset-Output-Variables.html

glitsj16
glitsj16 approved these changes Dec 4, 2022
View reviewed changes
Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kmk3
makefiles: stop overriding CFLAGS/LDFLAGS
87948b3 
From the manual of GNU Automake (version 1.16.5)[1] [2]:

> 3.6 Variables reserved for the user
>
> Some `Makefile` variables are reserved by the GNU Coding Standards for
> the use of the "user"—the person building the package.  For instance,
> `CFLAGS` is one such variable.
>
>    Sometimes package developers are tempted to set user variables such
> as `CFLAGS` because it appears to make their job easier.  However, the
> package itself should never set a user variable, particularly not to
> include switches that are required for proper compilation of the
> package.  Since these variables are documented as being for the
> package builder, that person rightfully expects to be able to override
> any of these variables at build time.
>
>    To get around this problem, Automake introduces an
> automake-specific shadow variable for each user flag variable.
> (Shadow variables are not introduced for variables like `CC`, where
> they would make no sense.) The shadow variable is named by prepending
> `AM_` to the user variable's name.  For instance, the shadow variable
> for `YFLAGS` is `AM_YFLAGS`.  The package maintainer—that is, the
> author(s) of the `Makefile.am` and `configure.ac` files—may adjust
> these shadow variables however necessary.
>
>    Note Flag Variables Ordering::, for more discussion about these
> variables and how they interact with per-target variables.

See also the description of CFLAGS in the GNU Autoconf manual[3].

Note: We do not use automake (save for aclocal) nor generally follow the
GNU Coding Standards, but the concept still applies.  Also, the closest
analogous in the project to the `AM_` prefix would currently likely be
`EXTRA_`.

[1] https://www.gnu.org/software/automake/manual/1.16.5/html_node/User-Variables.html
[2] https://www.gnu.org/software/automake/manual/1.16.5/html_node/Flag-Variables-Ordering.html
[3] https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Preset-Output-Variables.html
@kmk3
Copy link
Collaborator Author

kmk3 commented Dec 8, 2022

Force-pushed to remove commit e7be30a ("makefiles: set
EXTRA_CFLAGS/EXTRA_LDFLAGS instead of append", 2022-11-30).

It technically changes the behavior in more ways than intended (depending on
the usage) and makes more sense in a branch specific to dealing with
EXTRA_CFLAGS.

@kmk3 kmk3 merged commit 05e14ad into netblue30:master Dec 9, 2022
@kmk3 kmk3 deleted the build-cflags-improvements branch December 9, 2022 14:14
kmk3 added a commit that referenced this pull request Dec 19, 2022
@kmk3
RELNOTES: add build items
372f39d 
Relates to #5488 #5504.
@glitsj16 glitsj16 mentioned this pull request Mar 13, 2023
Closed
kmk3 added a commit to kmk3/firejail that referenced this pull request Jan 17, 2024
@kmk3
build: use CPPFLAGS instead of INCLUDE in compile targets
0c80ce1 
With this, CFLAGS and CPPFLAGS are used when compiling and LDFLAGS when
linking, just like in the built-in GNU make rules.  From `make -p`:

    COMPILE.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c
    LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH)
    LINK.o = $(CC) $(LDFLAGS) $(TARGET_ARCH)

Note: It is unclear where the `INCLUDE` variable comes from; it is not
documented in autoconf nor GNU make and automake (which itself is not
used in this repository) only mentions `INCLUDES`:

    `INCLUDES`
	 This does the same job as `AM_CPPFLAGS` (or any per-target
	 `_CPPFLAGS` variable if it is used).  It is an older name for
	 the same functionality.  This variable is deprecated; we
	 suggest using `AM_CPPFLAGS` and per-target `_CPPFLAGS` instead.

Environment: automake 1.16.5-2 and GNU make 4.4.1 on Artix Linux.

See also commit 671c3f2 ("build: actually set LDFLAGS and LIBS in
makefiles", 2022-11-30) / PR netblue30#5504.
kmk3 added a commit to kmk3/firejail that referenced this pull request Jan 17, 2024
@kmk3
build: use CPPFLAGS instead of INCLUDE in compile targets
54be68c 
With this, CFLAGS and CPPFLAGS are used when compiling and LDFLAGS when
linking, just like in the built-in GNU make rules.  From `make -p`:

    COMPILE.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c
    LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH)
    LINK.o = $(CC) $(LDFLAGS) $(TARGET_ARCH)

Note: It is unclear where the `INCLUDE` variable comes from; it is not
documented in autoconf nor GNU make and automake (which itself is not
used in this repository) only mentions `INCLUDES`:

    `INCLUDES`
	 This does the same job as `AM_CPPFLAGS` (or any per-target
	 `_CPPFLAGS` variable if it is used).  It is an older name for
	 the same functionality.  This variable is deprecated; we
	 suggest using `AM_CPPFLAGS` and per-target `_CPPFLAGS` instead.

Environment: automake 1.16.5-2 and GNU make 4.4.1 on Artix Linux.

See also commit 671c3f2 ("build: actually set LDFLAGS and LIBS in
makefiles", 2022-11-30) / PR netblue30#5504.
kmk3 added a commit to kmk3/firejail that referenced this pull request Jan 17, 2024
@kmk3
build: use CPPFLAGS instead of INCLUDE in compile targets
5b1ce32 
With this, CFLAGS and CPPFLAGS are used when compiling and LDFLAGS when
linking, just like in the built-in GNU make rules.  From `make -p`:

    COMPILE.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c
    LINK.c = $(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH)
    LINK.o = $(CC) $(LDFLAGS) $(TARGET_ARCH)

Note: It is unclear where the `INCLUDE` variable comes from; it is not
documented in autoconf nor GNU make and automake (which itself is not
used in this repository) only mentions `INCLUDES`:

    `INCLUDES`
         This does the same job as `AM_CPPFLAGS` (or any per-target
         `_CPPFLAGS` variable if it is used).  It is an older name for
         the same functionality.  This variable is deprecated; we
         suggest using `AM_CPPFLAGS` and per-target `_CPPFLAGS` instead.

Environment: automake 1.16.5-2 and GNU make 4.4.1 on Artix Linux.

See also commit 671c3f2 ("build: actually set LDFLAGS and LIBS in
makefiles", 2022-11-30) / PR netblue30#5504.
@kmk3 kmk3 mentioned this pull request Jan 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glitsj16 glitsj16 glitsj16 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Release 0.9.72
Status: Done (on RELNOTES)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kmk3 @glitsj16