F37 proposal: RPM Macros for Build Flags (System-Wide Change proposal)

[Ben Cotton]

https://fedoraproject.org/wiki/Changes/RPMMacrosForBuildFlags

This document represents a proposed Change. As part of the Changes
process, proposals are publicly announced in order to receive
community feedback. This proposal will only be implemented if approved
by the Fedora Engineering Steering Committee.

== Summary ==
Create a corresponding macro for each compiler flag in the
redhat-rpm-config macro file and create "extra flag" macros to make it
easier for packages to add and remove compiler flags.

== Owner ==
* Name: [[User:tstellar| Tom Stellard]]
* Email: <tstellar(a)redhat.com&gt;


== Detailed Description ==
The macros file in the redhat-rpm-config package contains a list of
default compiler flags for packages to use when compiling C, C++, and
Fortran packages.  There is currently no standard way to remove or add
to the set of default flags.  Most packages use a combination of echo
and sed to remove unwanted flags or add new ones.  Some examples:
    compiler-rt:
[https://src.fedoraproject.org/rpms/compiler-rt/blob/rawhide/f/compiler-rt...
global optflags %(echo %{optflags} -D_DEFAULT_SOURCE)]
    julia:
[https://src.fedoraproject.org/rpms/julia/blob/rawhide/f/julia.spec#_267
%global optflags %(echo %{optflags} | sed 's/-Wp,-D_GLIBCXX_ASSERTIONS
//')]

This change will add new macros which will make it easier for packages
to add and remove their own compiler flags.  This strategy is already
used to some extent with feature macros like %{_lto_cflags},
%{_hardening_cflags}, etc, but these new macros will give packagers
even more fine-grained control over the options.

The proposed macros for adding new flags are:

    %_pkg_extra_cflags
    %_pkg_extra_cxxflags
    %_pkg_extra_fflags
    %_pkg_extra_ldflags

These will be added to %{build_cflags}, %{build_cxxflags},
%{build_fflags}, and %{build_ldflags} respectively to allow packges to
add their own flags to the default list: e.g.

    %build_cflags %{optflags} %{_pkg_extra_cflags}

The proposed new macros to represent existing flags are:

    %_flag_fstack_protector_strong     -fstack-protector-strong
    %_flag_z_now                       -Wl,-z,now
    %_flag_z_defs                      -Wl,-z,defs
    %_flag_flto_auto                   -flto=auto
    %_flag_ffat_lto_objects            -ffat-lto-objects
    %_flag_o                           -O2
    %_flag_f_exceptions                -fexceptions
    %_flag_g                           -g
    %_flag_grecord_gcc_switches        -grecord-gcc-switches
    %_flag_pipe                        -pipe
    %_flag_wall                        -Wall
    %_flag_werror_format_security      -Werror=format-security
    %_flag_fortify_source              -Wp,-D_FORTIFY_SOURCE=2
    %_flag_glibcxx_assertions          -Wp,-D_GLIBCXX_ASSERTIONS
    %_flag_asynchronous_unwind_tables  -fasynchronous-unwind-tables
    %_flag_fstack_clash_protection     -fstack-clash-protection
    %_flag_fcf_protection              -fcf-protection
    %_flag_mbranch_protection_standard -mbranch-protection=standard

With these new macros, the examples from above could be re-written as:

    compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
    julia:       %global _flag_glibcxx_assertions %{nil}

For more details see the
[https://src.fedoraproject.org/fork/tstellar/rpms/redhat-rpm-config/c/0ee9...
Prototype Implementation].

In addition to adding these new macros, the packaging guidelines will
be updated to require that all new flags added to redhat-rpm-config
have their own RPM macro.


== Benefit to Fedora ==
* It will provide a standard way to disable existing compiler flags or
enable new ones that is more simple and robust than the existing echo
+ sed solution.
* It will make it easier to determine which packages disable or add
compiler flags by doing a simple grep of the spec files.
* It will make it easier for toolchain developers to experiment with
adding new flags to the distribution as this can be done with a simple
macro definition instead of patching redhat-rpm-config.


== Scope ==
* Proposal owners:
** Proposal owners will update the redhat-rpm-config package and add
the new macros.
** Proposal owners will test the changes to ensure that the correct
flags are still being used.

* Other developers:
** Other developers may, but are not required to, update their
packages to use the new macros.

* Release engineering: [https://pagure.io/releng/issues/10819 #10819]

* Policies and guidelines:
** The Fedora packaging policy will be updated to require that new
flags added to redhat-rpm-config come with their own RPM macro.

* Trademark approval: N/A (not needed for this Change)
* Alignment with Objectives:


== Upgrade/compatibility impact ==
None.


== How To Test ==
* This can be tested by inspecting the value of the %{build_cflags},
%{build_cxxflags}, %{build_fflags}, and %{build_ldflags} and ensuring
they are the same before and after the change.
* This can be tested by modifying some of the new macros in a spec
file and ensuring that the changes appear in the appropriate macro
mentioned above.


== User Experience ==
This is a change for developers and will have no impact to the user experience.


== Dependencies ==
None.


== Contingency Plan ==
* Contingency mechanism: (What to do?  Who will do it?) Change owner
will revert the update to redhat-rpm-config.
* Contingency deadline: Mass Rebuild
* Blocks release? N/A (not a System Wide Change), No

== Documentation ==
None.

== Release Notes ==
None.


-- 
Ben Cotton
He / Him / His
Fedora Program Manager
Red Hat
TZ=America/Indiana/Indianapolis

[Vít Ondruch]

Hi Tom,

Since you are looking into this and I like this proposal, have you 
considered to look alto into `%extension_*flags`:

https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/macro...

This is longstanding issue:

https://bugzilla.redhat.com/1284684

Where we have several proposals for fix, but non of them is really 
appealing to me:

https://src.fedoraproject.org/rpms/ruby/pull-request/110

https://src.fedoraproject.org/rpms/ruby/pull-request/117


BTW isn't the `_flag_` prefix too generic? And also, the initial 
underscore implies that this is internal macro which should ideally not 
be used. So should it be rather removed or not?


Vít


Dne 02. 06. 22 v 21:27 Ben Cotton napsal(a):
>  https://fedoraproject.org/wiki/Changes/RPMMacrosForBuildFlags
>
>  This document represents a proposed Change. As part of the Changes
>  process, proposals are publicly announced in order to receive
>  community feedback. This proposal will only be implemented if approved
>  by the Fedora Engineering Steering Committee.
>
>  == Summary ==
>  Create a corresponding macro for each compiler flag in the
>  redhat-rpm-config macro file and create "extra flag" macros to make it
>  easier for packages to add and remove compiler flags.
>
>  == Owner ==
>  * Name: [[User:tstellar| Tom Stellard]]
>  * Email: <tstellar(a)redhat.com&gt;
>
>
>  == Detailed Description ==
>  The macros file in the redhat-rpm-config package contains a list of
>  default compiler flags for packages to use when compiling C, C++, and
>  Fortran packages.  There is currently no standard way to remove or add
>  to the set of default flags.  Most packages use a combination of echo
>  and sed to remove unwanted flags or add new ones.  Some examples:
>       compiler-rt:
>  [https://src.fedoraproject.org/rpms/compiler-rt/blob/rawhide/f/compiler-rt...
>  global optflags %(echo %{optflags} -D_DEFAULT_SOURCE)]
>       julia:
>  [https://src.fedoraproject.org/rpms/julia/blob/rawhide/f/julia.spec#_267
>  %global optflags %(echo %{optflags} | sed 's/-Wp,-D_GLIBCXX_ASSERTIONS
>  //')]
>
>  This change will add new macros which will make it easier for packages
>  to add and remove their own compiler flags.  This strategy is already
>  used to some extent with feature macros like %{_lto_cflags},
>  %{_hardening_cflags}, etc, but these new macros will give packagers
>  even more fine-grained control over the options.
>
>  The proposed macros for adding new flags are:
>
>       %_pkg_extra_cflags
>       %_pkg_extra_cxxflags
>       %_pkg_extra_fflags
>       %_pkg_extra_ldflags
>
>  These will be added to %{build_cflags}, %{build_cxxflags},
>  %{build_fflags}, and %{build_ldflags} respectively to allow packges to
>  add their own flags to the default list: e.g.
>
>       %build_cflags %{optflags} %{_pkg_extra_cflags}
>
>  The proposed new macros to represent existing flags are:
>
>       %_flag_fstack_protector_strong     -fstack-protector-strong
>       %_flag_z_now                       -Wl,-z,now
>       %_flag_z_defs                      -Wl,-z,defs
>       %_flag_flto_auto                   -flto=auto
>       %_flag_ffat_lto_objects            -ffat-lto-objects
>       %_flag_o                           -O2
>       %_flag_f_exceptions                -fexceptions
>       %_flag_g                           -g
>       %_flag_grecord_gcc_switches        -grecord-gcc-switches
>       %_flag_pipe                        -pipe
>       %_flag_wall                        -Wall
>       %_flag_werror_format_security      -Werror=format-security
>       %_flag_fortify_source              -Wp,-D_FORTIFY_SOURCE=2
>       %_flag_glibcxx_assertions          -Wp,-D_GLIBCXX_ASSERTIONS
>       %_flag_asynchronous_unwind_tables  -fasynchronous-unwind-tables
>       %_flag_fstack_clash_protection     -fstack-clash-protection
>       %_flag_fcf_protection              -fcf-protection
>       %_flag_mbranch_protection_standard -mbranch-protection=standard
>
>  With these new macros, the examples from above could be re-written as:
>
>       compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
>       julia:       %global _flag_glibcxx_assertions %{nil}
>
>  For more details see the
>
> [https://src.fedoraproject.org/fork/tstellar/rpms/redhat-rpm-config/c/0ee9...
>  Prototype Implementation].
>
>  In addition to adding these new macros, the packaging guidelines will
>  be updated to require that all new flags added to redhat-rpm-config
>  have their own RPM macro.
>
>
>  == Benefit to Fedora ==
>  * It will provide a standard way to disable existing compiler flags or
>  enable new ones that is more simple and robust than the existing echo
>  + sed solution.
>  * It will make it easier to determine which packages disable or add
>  compiler flags by doing a simple grep of the spec files.
>  * It will make it easier for toolchain developers to experiment with
>  adding new flags to the distribution as this can be done with a simple
>  macro definition instead of patching redhat-rpm-config.
>
>
>  == Scope ==
>  * Proposal owners:
>  ** Proposal owners will update the redhat-rpm-config package and add
>  the new macros.
>  ** Proposal owners will test the changes to ensure that the correct
>  flags are still being used.
>
>  * Other developers:
>  ** Other developers may, but are not required to, update their
>  packages to use the new macros.
>
>  * Release engineering: [https://pagure.io/releng/issues/10819 #10819]
>
>  * Policies and guidelines:
>  ** The Fedora packaging policy will be updated to require that new
>  flags added to redhat-rpm-config come with their own RPM macro.
>
>  * Trademark approval: N/A (not needed for this Change)
>  * Alignment with Objectives:
>
>
>  == Upgrade/compatibility impact ==
>  None.
>
>
>  == How To Test ==
>  * This can be tested by inspecting the value of the %{build_cflags},
>  %{build_cxxflags}, %{build_fflags}, and %{build_ldflags} and ensuring
>  they are the same before and after the change.
>  * This can be tested by modifying some of the new macros in a spec
>  file and ensuring that the changes appear in the appropriate macro
>  mentioned above.
>
>
>  == User Experience ==
>  This is a change for developers and will have no impact to the user experience.
>
>
>  == Dependencies ==
>  None.
>
>
>  == Contingency Plan ==
>  * Contingency mechanism: (What to do?  Who will do it?) Change owner
>  will revert the update to redhat-rpm-config.
>  * Contingency deadline: Mass Rebuild
>  * Blocks release? N/A (not a System Wide Change), No
>
>  == Documentation ==
>  None.
>
>  == Release Notes ==
>  None.
>
>
>

[Fabio Valentini]

On Fri, Jun 3, 2022 at 11:25 AM Vít Ondruch <vondruch(a)redhat.com&gt; wrote:
>  BTW isn't the `_flag_` prefix too generic? And also, the initial
>  underscore implies that this is internal macro which should ideally not
>  be used. So should it be rather removed or not? 
I agree that the "_flag_" prefix might be a little too generic, but
what would be a better alternative?
Maybe something like _optflag_, to match what they are "collected
into" (i.e. %optflags)?

Also, macro names with single leading underscores are *fine* (see also
%_bindir, %_libdir, %_datadir, etc.).
Those with *double* leading underscores are the ones that should be
considered "internal" implementation details.

Fabio

[Tom Stellard]

On 6/3/22 03:43, Fabio Valentini wrote:
>  On Fri, Jun 3, 2022 at 11:25 AM Vít Ondruch
> <vondruch(a)redhat.com&gt; wrote:
> >
> > BTW isn't the `_flag_` prefix too generic? And also, the initial
> > underscore implies that this is internal macro which should ideally not
> > be used. So should it be rather removed or not?
>  
>  I agree that the "_flag_" prefix might be a little too generic, but
>  what would be a better alternative?
>  Maybe something like _optflag_, to match what they are "collected
>  into" (i.e. %optflags)? 
What about prefixing them with _build_flag_ The redhat-rpm-config docs[1],
recommend using the %build_*flags macros instead of %optflags, so maybe we
should try to match that.

[1] https://src.fedoraproject.org/rpms/redhat-rpm-config//blob/rawhide/f/buil...

-Tom

>  
>  Also, macro names with single leading underscores are *fine* (see also
>  %_bindir, %_libdir, %_datadir, etc.).
>  Those with *double* leading underscores are the ones that should be
>  considered "internal" implementation details.
>  
>  Fabio
>  _______________________________________________
>  devel mailing list -- devel(a)lists.fedoraproject.org
>  To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedoraproject.org/archives/list/[e-mail]
>  Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Panu Matilainen]

On 6/3/22 13:43, Fabio Valentini wrote:
>  On Fri, Jun 3, 2022 at 11:25 AM Vít Ondruch
> <vondruch(a)redhat.com&gt; wrote:
> >
> > BTW isn't the `_flag_` prefix too generic? And also, the initial
> > underscore implies that this is internal macro which should ideally not
> > be used. So should it be rather removed or not?
>  
>  I agree that the "_flag_" prefix might be a little too generic, but
>  what would be a better alternative?
>  Maybe something like _optflag_, to match what they are "collected
>  into" (i.e. %optflags)?
>  
>  Also, macro names with single leading underscores are *fine* (see also
>  %_bindir, %_libdir, %_datadir, etc.).
>  Those with *double* leading underscores are the ones that should be
>  considered "internal" implementation details. 
Once upon a time in past I can still remember, the following rule of 
thumb for macro underscores was set [1]:

	Use %__foo to set, %foo to get.

To me it looks like the entire set of suggested flags is basically 
write-only values, and thus should have two leading underscores. So, 
%__build_flag_whatever. Usage should always happen through the 
non-underscored %build_cflags and friends, which can do their own 
internal logic around this stuff, use Y only if X is enabled.

Other misc comments:

>  %_flag_flto_auto                   -flto=auto 
Shouldn't this be %_flag_flto instead (or rather, %__build_flag_flto), 
just like %_flag_o does not carry the optimization level in the name?

>   %_flag_werror_format_security      -Werror=format-security 
...and ditto for this, %__build_flag_werror whose default value is 
-Werror=format-security ... except that unlike -flto, -Werror can appear 
multiple times. Dunno.

What I guess I'm after, having an actual rule for the parameter -> macro 
naming, one that could preferably be automated, would be beneficial. The 
-Wl and -Wp related macro naming would need further consideration wrt that.

%_flag_pipe seems like the odd man out there because it doesn't actually 
relate to code at all, but I guess consistency is the goal there.

[1] https://pagure.io/packaging-committee/issue/907

	- Panu -

[Tom Stellard]

On 6/6/22 00:58, Panu Matilainen wrote:
>  On 6/3/22 13:43, Fabio Valentini wrote:
> > On Fri, Jun 3, 2022 at 11:25 AM Vít Ondruch <vondruch(a)redhat.com&gt; wrote:
> >>
> >> BTW isn't the `_flag_` prefix too generic? And also, the initial
> >> underscore implies that this is internal macro which should ideally not
> >> be used. So should it be rather removed or not?
> >
> > I agree that the "_flag_" prefix might be a little too generic, but
> > what would be a better alternative?
> > Maybe something like _optflag_, to match what they are "collected
> > into" (i.e. %optflags)?
> >
> > Also, macro names with single leading underscores are *fine* (see also
> > %_bindir, %_libdir, %_datadir, etc.).
> > Those with *double* leading underscores are the ones that should be
> > considered "internal" implementation details.
>  
>  Once upon a time in past I can still remember, the following rule of thumb for macro
> underscores was set [1]:
>  
>       Use %__foo to set, %foo to get.
>  
>  To me it looks like the entire set of suggested flags is basically write-only values, and
> thus should have two leading underscores. So, %__build_flag_whatever. Usage should always
> happen through the non-underscored %build_cflags and friends, which can do their own
> internal logic around this stuff, use Y only if X is enabled.
>   
I don't have a preference on one vs two underscore.  I chose one, because
that's what most of the existing compiler flag macros use.

>  Other misc comments:
>  
> > %_flag_flto_auto                   -flto=auto
>  
>  Shouldn't this be %_flag_flto instead (or rather, %__build_flag_flto), just like
> %_flag_o does not carry the optimization level in the name?
>   
OK, I think that makes sense.

> >  %_flag_werror_format_security      -Werror=format-security
>  
>  ...and ditto for this, %__build_flag_werror whose default value is
> -Werror=format-security ... except that unlike -flto, -Werror can appear multiple times.
> Dunno.
>  
>  What I guess I'm after, having an actual rule for the parameter -> macro naming,
> one that could preferably be automated, would be beneficial. The -Wl and -Wp related macro
> naming would need further consideration wrt that.
>   
I don't think there is a way to have the consistent naming that you
are looking for.  Even though -Werror= usage the same format as -flto=
and others, each -Werror= option is really its own separate option.

-Tom

>  %_flag_pipe seems like the odd man out there because it doesn't
> actually relate to code at all, but I guess consistency is the goal there.
>  
>  [1] https://pagure.io/packaging-committee/issue/907
>  
>       - Panu -
>  _______________________________________________
>  devel mailing list -- devel(a)lists.fedoraproject.org
>  To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedoraproject.org/archives/list/[e-mail]
>  Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Tom Stellard]

On 6/3/22 02:24, Vít Ondruch wrote:
>  Hi Tom,
>  
>  Since you are looking into this and I like this proposal, have you considered to look
> alto into `%extension_*flags`:
>  
>  https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/macro...
>   
I have not considered this.  Do you think there is some way this proposal
could be extended to help solve this problem as well?

-Tom

>  This is longstanding issue:
>  
>  https://bugzilla.redhat.com/1284684
>  
>  Where we have several proposals for fix, but non of them is really appealing to me:
>  
>  https://src.fedoraproject.org/rpms/ruby/pull-request/110
>  
>  https://src.fedoraproject.org/rpms/ruby/pull-request/117
>  
>  
>  BTW isn't the `_flag_` prefix too generic? And also, the initial underscore implies
> that this is internal macro which should ideally not be used. So should it be rather
> removed or not?
>  
>  
>  Vít
>  
>  
>  Dne 02. 06. 22 v 21:27 Ben Cotton napsal(a):
> > https://fedoraproject.org/wiki/Changes/RPMMacrosForBuildFlags
> >
> > This document represents a proposed Change. As part of the Changes
> > process, proposals are publicly announced in order to receive
> > community feedback. This proposal will only be implemented if approved
> > by the Fedora Engineering Steering Committee.
> >
> > == Summary ==
> > Create a corresponding macro for each compiler flag in the
> > redhat-rpm-config macro file and create "extra flag" macros to make it
> > easier for packages to add and remove compiler flags.
> >
> > == Owner ==
> > * Name: [[User:tstellar| Tom Stellard]]
> > * Email: <tstellar(a)redhat.com&gt;
> >
> >
> > == Detailed Description ==
> > The macros file in the redhat-rpm-config package contains a list of
> > default compiler flags for packages to use when compiling C, C++, and
> > Fortran packages.  There is currently no standard way to remove or add
> > to the set of default flags.  Most packages use a combination of echo
> > and sed to remove unwanted flags or add new ones.  Some examples:
> >      compiler-rt:
> > [https://src.fedoraproject.org/rpms/compiler-rt/blob/rawhide/f/compiler-rt...
> > global optflags %(echo %{optflags} -D_DEFAULT_SOURCE)]
> >      julia:
> > [https://src.fedoraproject.org/rpms/julia/blob/rawhide/f/julia.spec#_267
> > %global optflags %(echo %{optflags} | sed 's/-Wp,-D_GLIBCXX_ASSERTIONS
> > //')]
> >
> > This change will add new macros which will make it easier for packages
> > to add and remove their own compiler flags.  This strategy is already
> > used to some extent with feature macros like %{_lto_cflags},
> > %{_hardening_cflags}, etc, but these new macros will give packagers
> > even more fine-grained control over the options.
> >
> > The proposed macros for adding new flags are:
> >
> >      %_pkg_extra_cflags
> >      %_pkg_extra_cxxflags
> >      %_pkg_extra_fflags
> >      %_pkg_extra_ldflags
> >
> > These will be added to %{build_cflags}, %{build_cxxflags},
> > %{build_fflags}, and %{build_ldflags} respectively to allow packges to
> > add their own flags to the default list: e.g.
> >
> >      %build_cflags %{optflags} %{_pkg_extra_cflags}
> >
> > The proposed new macros to represent existing flags are:
> >
> >      %_flag_fstack_protector_strong     -fstack-protector-strong
> >      %_flag_z_now                       -Wl,-z,now
> >      %_flag_z_defs                      -Wl,-z,defs
> >      %_flag_flto_auto                   -flto=auto
> >      %_flag_ffat_lto_objects            -ffat-lto-objects
> >      %_flag_o                           -O2
> >      %_flag_f_exceptions                -fexceptions
> >      %_flag_g                           -g
> >      %_flag_grecord_gcc_switches        -grecord-gcc-switches
> >      %_flag_pipe                        -pipe
> >      %_flag_wall                        -Wall
> >      %_flag_werror_format_security      -Werror=format-security
> >      %_flag_fortify_source              -Wp,-D_FORTIFY_SOURCE=2
> >      %_flag_glibcxx_assertions          -Wp,-D_GLIBCXX_ASSERTIONS
> >      %_flag_asynchronous_unwind_tables  -fasynchronous-unwind-tables
> >      %_flag_fstack_clash_protection     -fstack-clash-protection
> >      %_flag_fcf_protection              -fcf-protection
> >      %_flag_mbranch_protection_standard -mbranch-protection=standard
> >
> > With these new macros, the examples from above could be re-written as:
> >
> >      compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
> >      julia:       %global _flag_glibcxx_assertions %{nil}
> >
> > For more details see the
> >
> [https://src.fedoraproject.org/fork/tstellar/rpms/redhat-rpm-config/c/0ee9...
> > Prototype Implementation].
> >
> > In addition to adding these new macros, the packaging guidelines will
> > be updated to require that all new flags added to redhat-rpm-config
> > have their own RPM macro.
> >
> >
> > == Benefit to Fedora ==
> > * It will provide a standard way to disable existing compiler flags or
> > enable new ones that is more simple and robust than the existing echo
> > + sed solution.
> > * It will make it easier to determine which packages disable or add
> > compiler flags by doing a simple grep of the spec files.
> > * It will make it easier for toolchain developers to experiment with
> > adding new flags to the distribution as this can be done with a simple
> > macro definition instead of patching redhat-rpm-config.
> >
> >
> > == Scope ==
> > * Proposal owners:
> > ** Proposal owners will update the redhat-rpm-config package and add
> > the new macros.
> > ** Proposal owners will test the changes to ensure that the correct
> > flags are still being used.
> >
> > * Other developers:
> > ** Other developers may, but are not required to, update their
> > packages to use the new macros.
> >
> > * Release engineering: [https://pagure.io/releng/issues/10819 #10819]
> >
> > * Policies and guidelines:
> > ** The Fedora packaging policy will be updated to require that new
> > flags added to redhat-rpm-config come with their own RPM macro.
> >
> > * Trademark approval: N/A (not needed for this Change)
> > * Alignment with Objectives:
> >
> >
> > == Upgrade/compatibility impact ==
> > None.
> >
> >
> > == How To Test ==
> > * This can be tested by inspecting the value of the %{build_cflags},
> > %{build_cxxflags}, %{build_fflags}, and %{build_ldflags} and ensuring
> > they are the same before and after the change.
> > * This can be tested by modifying some of the new macros in a spec
> > file and ensuring that the changes appear in the appropriate macro
> > mentioned above.
> >
> >
> > == User Experience ==
> > This is a change for developers and will have no impact to the user experience.
> >
> >
> > == Dependencies ==
> > None.
> >
> >
> > == Contingency Plan ==
> > * Contingency mechanism: (What to do?  Who will do it?) Change owner
> > will revert the update to redhat-rpm-config.
> > * Contingency deadline: Mass Rebuild
> > * Blocks release? N/A (not a System Wide Change), No
> >
> > == Documentation ==
> > None.
> >
> > == Release Notes ==
> > None.
> >
> >
>  
>  _______________________________________________
>  devel mailing list -- devel(a)lists.fedoraproject.org
>  To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedoraproject.org/archives/list/[e-mail]
>  Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Vít Ondruch]

Dne 03. 06. 22 v 16:32 Tom Stellard napsal(a):
>  On 6/3/22 02:24, Vít Ondruch wrote:
> > Hi Tom,
> >
> > Since you are looking into this and I like this proposal, have you 
> > considered to look alto into `%extension_*flags`:
> >
> > https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/macro...
>
> >
> >
>
>  I have not considered this.  Do you think there is some way this proposal
>  could be extended to help solve this problem as well?
>  

I think the current struggle is that we have `%extension_cflags` and 
`%build_cflags`, while we would actually need the set of ``%build_cflags 
- %extension_cflags`. The problem is that the `%build_cflags` order is 
not necessarily followed in the resulting code, so if we need to modify 
the resulting code, we have no way to achieve this.


Vít


>  -Tom
>
> > This is longstanding issue:
> >
> > https://bugzilla.redhat.com/1284684
> >
> > Where we have several proposals for fix, but non of them is really 
> > appealing to me:
> >
> > https://src.fedoraproject.org/rpms/ruby/pull-request/110
> >
> > https://src.fedoraproject.org/rpms/ruby/pull-request/117
> >
> >
> > BTW isn't the `_flag_` prefix too generic? And also, the initial 
> > underscore implies that this is internal macro which should ideally 
> > not be used. So should it be rather removed or not?
> >
> >
> > Vít
> >
> >
> > Dne 02. 06. 22 v 21:27 Ben Cotton napsal(a):
> >> https://fedoraproject.org/wiki/Changes/RPMMacrosForBuildFlags
> >>
> >> This document represents a proposed Change. As part of the Changes
> >> process, proposals are publicly announced in order to receive
> >> community feedback. This proposal will only be implemented if approved
> >> by the Fedora Engineering Steering Committee.
> >>
> >> == Summary ==
> >> Create a corresponding macro for each compiler flag in the
> >> redhat-rpm-config macro file and create "extra flag" macros to make
> it
> >> easier for packages to add and remove compiler flags.
> >>
> >> == Owner ==
> >> * Name: [[User:tstellar| Tom Stellard]]
> >> * Email: <tstellar(a)redhat.com&gt;
> >>
> >>
> >> == Detailed Description ==
> >> The macros file in the redhat-rpm-config package contains a list of
> >> default compiler flags for packages to use when compiling C, C++, and
> >> Fortran packages.  There is currently no standard way to remove or add
> >> to the set of default flags.  Most packages use a combination of echo
> >> and sed to remove unwanted flags or add new ones.  Some examples:
> >>      compiler-rt:
> >>
> [https://src.fedoraproject.org/rpms/compiler-rt/blob/rawhide/f/compiler-rt... 
> >>
> >> global optflags %(echo %{optflags} -D_DEFAULT_SOURCE)]
> >>      julia:
> >> [https://src.fedoraproject.org/rpms/julia/blob/rawhide/f/julia.spec#_267 
> >>
> >> %global optflags %(echo %{optflags} | sed 's/-Wp,-D_GLIBCXX_ASSERTIONS
> >> //')]
> >>
> >> This change will add new macros which will make it easier for packages
> >> to add and remove their own compiler flags.  This strategy is already
> >> used to some extent with feature macros like %{_lto_cflags},
> >> %{_hardening_cflags}, etc, but these new macros will give packagers
> >> even more fine-grained control over the options.
> >>
> >> The proposed macros for adding new flags are:
> >>
> >>      %_pkg_extra_cflags
> >>      %_pkg_extra_cxxflags
> >>      %_pkg_extra_fflags
> >>      %_pkg_extra_ldflags
> >>
> >> These will be added to %{build_cflags}, %{build_cxxflags},
> >> %{build_fflags}, and %{build_ldflags} respectively to allow packges to
> >> add their own flags to the default list: e.g.
> >>
> >>      %build_cflags %{optflags} %{_pkg_extra_cflags}
> >>
> >> The proposed new macros to represent existing flags are:
> >>
> >>      %_flag_fstack_protector_strong -fstack-protector-strong
> >>      %_flag_z_now                       -Wl,-z,now
> >>      %_flag_z_defs                      -Wl,-z,defs
> >>      %_flag_flto_auto                   -flto=auto
> >>      %_flag_ffat_lto_objects            -ffat-lto-objects
> >>      %_flag_o                           -O2
> >>      %_flag_f_exceptions                -fexceptions
> >>      %_flag_g                           -g
> >>      %_flag_grecord_gcc_switches        -grecord-gcc-switches
> >>      %_flag_pipe                        -pipe
> >>      %_flag_wall                        -Wall
> >>      %_flag_werror_format_security -Werror=format-security
> >>      %_flag_fortify_source -Wp,-D_FORTIFY_SOURCE=2
> >>      %_flag_glibcxx_assertions -Wp,-D_GLIBCXX_ASSERTIONS
> >>      %_flag_asynchronous_unwind_tables -fasynchronous-unwind-tables
> >>      %_flag_fstack_clash_protection -fstack-clash-protection
> >>      %_flag_fcf_protection              -fcf-protection
> >>      %_flag_mbranch_protection_standard -mbranch-protection=standard
> >>
> >> With these new macros, the examples from above could be re-written as:
> >>
> >>      compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
> >>      julia:       %global _flag_glibcxx_assertions %{nil}
> >>
> >> For more details see the
> >>
> [https://src.fedoraproject.org/fork/tstellar/rpms/redhat-rpm-config/c/0ee9...
>
> >>
> >> Prototype Implementation].
> >>
> >> In addition to adding these new macros, the packaging guidelines will
> >> be updated to require that all new flags added to redhat-rpm-config
> >> have their own RPM macro.
> >>
> >>
> >> == Benefit to Fedora ==
> >> * It will provide a standard way to disable existing compiler flags or
> >> enable new ones that is more simple and robust than the existing echo
> >> + sed solution.
> >> * It will make it easier to determine which packages disable or add
> >> compiler flags by doing a simple grep of the spec files.
> >> * It will make it easier for toolchain developers to experiment with
> >> adding new flags to the distribution as this can be done with a simple
> >> macro definition instead of patching redhat-rpm-config.
> >>
> >>
> >> == Scope ==
> >> * Proposal owners:
> >> ** Proposal owners will update the redhat-rpm-config package and add
> >> the new macros.
> >> ** Proposal owners will test the changes to ensure that the correct
> >> flags are still being used.
> >>
> >> * Other developers:
> >> ** Other developers may, but are not required to, update their
> >> packages to use the new macros.
> >>
> >> * Release engineering: [https://pagure.io/releng/issues/10819 #10819]
> >>
> >> * Policies and guidelines:
> >> ** The Fedora packaging policy will be updated to require that new
> >> flags added to redhat-rpm-config come with their own RPM macro.
> >>
> >> * Trademark approval: N/A (not needed for this Change)
> >> * Alignment with Objectives:
> >>
> >>
> >> == Upgrade/compatibility impact ==
> >> None.
> >>
> >>
> >> == How To Test ==
> >> * This can be tested by inspecting the value of the %{build_cflags},
> >> %{build_cxxflags}, %{build_fflags}, and %{build_ldflags} and ensuring
> >> they are the same before and after the change.
> >> * This can be tested by modifying some of the new macros in a spec
> >> file and ensuring that the changes appear in the appropriate macro
> >> mentioned above.
> >>
> >>
> >> == User Experience ==
> >> This is a change for developers and will have no impact to the user 
> >> experience.
> >>
> >>
> >> == Dependencies ==
> >> None.
> >>
> >>
> >> == Contingency Plan ==
> >> * Contingency mechanism: (What to do?  Who will do it?) Change owner
> >> will revert the update to redhat-rpm-config.
> >> * Contingency deadline: Mass Rebuild
> >> * Blocks release? N/A (not a System Wide Change), No
> >>
> >> == Documentation ==
> >> None.
> >>
> >> == Release Notes ==
> >> None.
> >>
> >>
> >
> > _______________________________________________
> > devel mailing list -- devel(a)lists.fedoraproject.org
> > To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
> > Fedora Code of Conduct: 
> > https://docs.fedoraproject.org/en-US/project/code-of-conduct/
> > List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
> > List Archives: 
> > https://lists.fedoraproject.org/archives/list/[e-mail]
> > Do not reply to spam on the list, report it: 
> > https://pagure.io/fedora-infrastructure
>
>

[Florian Weimer]

* Vít Ondruch:

>  Dne 03. 06. 22 v 16:32 Tom Stellard napsal(a):
> > On 6/3/22 02:24, Vít Ondruch wrote:
> >> Hi Tom,
> >>
> >> Since you are looking into this and I like this proposal, have you
> >> considered to look alto into `%extension_*flags`:
> >>
> >>
> https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/macro... 
> >>
> >
> > I have not considered this.  Do you think there is some way this proposal
> > could be extended to help solve this problem as well?
>
>
>  I think the current struggle is that we have `%extension_cflags` and
>  `%build_cflags`, while we would actually need the set of
>  ``%build_cflags - %extension_cflags`. The problem is that the
>  `%build_cflags` order is not necessarily followed in the resulting
>  code, so if we need to modify the resulting code, we have no way to
>  achieve this. 
I don't see how the list difference would solve your fundamental issue.
Could you elaborate?  Or maybe you already did, and you missed it, in
which case I would appreciate a pointer.

Thanks,
Florian

[Vít Ondruch]

Dne 07. 06. 22 v 11:10 Florian Weimer napsal(a):
>  * Vít Ondruch:
>
> > Dne 03. 06. 22 v 16:32 Tom Stellard napsal(a):
> >> On 6/3/22 02:24, Vít Ondruch wrote:
> >>> Hi Tom,
> >>>
> >>> Since you are looking into this and I like this proposal, have you
> >>> considered to look alto into `%extension_*flags`:
> >>>
> >>>
> https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/macro...
> >>>
> >> I have not considered this.  Do you think there is some way this proposal
> >> could be extended to help solve this problem as well?
> >
> > I think the current struggle is that we have `%extension_cflags` and
> > `%build_cflags`, while we would actually need the set of
> > ``%build_cflags - %extension_cflags`. The problem is that the
> > `%build_cflags` order is not necessarily followed in the resulting
> > code, so if we need to modify the resulting code, we have no way to
> > achieve this.
>  I don't see how the list difference would solve your fundamental issue.
>  Could you elaborate?  Or maybe you already did, and you missed it, in
>  which case I would appreciate a pointer.
>  

https://src.fedoraproject.org/rpms/ruby/pull-request/110

This PR ^^ has the lengthy discussion. But let me reiterate. So there is 
`%build_cflags` and the original assumption was that if you give these 
flags to the build system, they are recorded also in the output, so you 
could do something such as `s/%build_cflags/%extension_cflags/`. But 
that does not work, because the build system is free to change the 
`%build_cflags`. So in the output, there is something we can call e.g. 
`%output_build_cflags`. From these flags, we would like to remove the 
flags which are not usable for the extensions, i.e. the set of 
`%build_cflags - %extension_cflags`.

IOW, these are the `%build_cflags`:

~~~

$ rpm --eval '%build_cflags'
-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches 
-pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 
-Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 
-fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  
-m64 -mtune=generic -fasynchronous-unwind-tables 
-fstack-clash-protection -fcf-protection

~~~

while these are the output flags:

~~~

-O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches 
-pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 
-Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 
-fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  
-mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection 
-fcf-protection -fPIC

~~~

If we had the `%build_cflags - %extension_cflags` set, we could at least 
iterate over the set and remove the flags from the output.


Vít



>  Thanks,
>  Florian
>
>

[Otto Urpelainen]

Ben Cotton kirjoitti 2.6.2022 klo 22.27:
>  * Policies and guidelines:
>  ** The Fedora packaging policy will be updated to require that new
>  flags added to redhat-rpm-config come with their own RPM macro. 
By the proposal owners, I presume? The guidelines should also be updated 
to present the new macros as the preferred way of modifying build flags.

[Florian Weimer]

* Ben Cotton:

>  This change will add new macros which will make it easier for
> packages
>  to add and remove their own compiler flags.  This strategy is already
>  used to some extent with feature macros like %{_lto_cflags},
>  %{_hardening_cflags}, etc, but these new macros will give packagers
>  even more fine-grained control over the options.
>
>  The proposed macros for adding new flags are:
>
>      %_pkg_extra_cflags
>      %_pkg_extra_cxxflags
>      %_pkg_extra_fflags
>      %_pkg_extra_ldflags 
Why isn't it possible to use the environment variables for this?

>  The proposed new macros to represent existing flags are:
>
>      %_flag_fstack_protector_strong     -fstack-protector-strong
>      %_flag_z_now                       -Wl,-z,now
>      %_flag_z_defs                      -Wl,-z,defs
>      %_flag_flto_auto                   -flto=auto
>      %_flag_ffat_lto_objects            -ffat-lto-objects
>      %_flag_o                           -O2
>      %_flag_f_exceptions                -fexceptions
>      %_flag_g                           -g
>      %_flag_grecord_gcc_switches        -grecord-gcc-switches
>      %_flag_pipe                        -pipe
>      %_flag_wall                        -Wall
>      %_flag_werror_format_security      -Werror=format-security
>      %_flag_fortify_source              -Wp,-D_FORTIFY_SOURCE=2
>      %_flag_glibcxx_assertions          -Wp,-D_GLIBCXX_ASSERTIONS
>      %_flag_asynchronous_unwind_tables  -fasynchronous-unwind-tables
>      %_flag_fstack_clash_protection     -fstack-clash-protection
>      %_flag_fcf_protection              -fcf-protection
>      %_flag_mbranch_protection_standard -mbranch-protection=standard 
This is very misleading because in several cases, clearing the those
flags will not affect toolchain behavior because the flag in question
merely restates the toolchain default.  In particular, this applies to
-fasynchronous-unwind-tables, and it would apply to -march= and -mcpu=
if those were in the list.  Likewise to -Wl,-z,relro.

Is the goal of this proposal just to achieve a textual flags change
(disregarding any change in behavior), or to actually change toolchain
behavior?

If the former, I'm not sure if the actual _flag names are useful.  Maybe
we can add an RPM macro to suppress or replace flags based on the flags
as they are used instead.  This could also add additional error checking
because a name typo in the macro definition will not be immediately
obvious.

>  With these new macros, the examples from above could be re-written
> as:
>
>      compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
>      julia:       %global _flag_glibcxx_assertions %{nil} 
Do you have some background why -D_DEFAULT_SOURCE is needed?  Why
doesn't upstream detect this?

Thanks,
Florian

[Tom Stellard]

On 6/7/22 02:18, Florian Weimer wrote:
>  * Ben Cotton:
>  
> > This change will add new macros which will make it easier for packages
> > to add and remove their own compiler flags.  This strategy is already
> > used to some extent with feature macros like %{_lto_cflags},
> > %{_hardening_cflags}, etc, but these new macros will give packagers
> > even more fine-grained control over the options.
> >
> > The proposed macros for adding new flags are:
> >
> >      %_pkg_extra_cflags
> >      %_pkg_extra_cxxflags
> >      %_pkg_extra_fflags
> >      %_pkg_extra_ldflags
>  
>  Why isn't it possible to use the environment variables for this?
>   
Environment variables won't cover all cases.  For example, some packages
need the flags passed as arguments to make or configure or some other
build system.

> > The proposed new macros to represent existing flags are:
> >
> >      %_flag_fstack_protector_strong     -fstack-protector-strong
> >      %_flag_z_now                       -Wl,-z,now
> >      %_flag_z_defs                      -Wl,-z,defs
> >      %_flag_flto_auto                   -flto=auto
> >      %_flag_ffat_lto_objects            -ffat-lto-objects
> >      %_flag_o                           -O2
> >      %_flag_f_exceptions                -fexceptions
> >      %_flag_g                           -g
> >      %_flag_grecord_gcc_switches        -grecord-gcc-switches
> >      %_flag_pipe                        -pipe
> >      %_flag_wall                        -Wall
> >      %_flag_werror_format_security      -Werror=format-security
> >      %_flag_fortify_source              -Wp,-D_FORTIFY_SOURCE=2
> >      %_flag_glibcxx_assertions          -Wp,-D_GLIBCXX_ASSERTIONS
> >      %_flag_asynchronous_unwind_tables  -fasynchronous-unwind-tables
> >      %_flag_fstack_clash_protection     -fstack-clash-protection
> >      %_flag_fcf_protection              -fcf-protection
> >      %_flag_mbranch_protection_standard -mbranch-protection=standard
>  
>  This is very misleading because in several cases, clearing the those
>  flags will not affect toolchain behavior because the flag in question
>  merely restates the toolchain default.  In particular, this applies to
>  -fasynchronous-unwind-tables, and it would apply to -march= and -mcpu=
>  if those were in the list.  Likewise to -Wl,-z,relro.
>  
>  Is the goal of this proposal just to achieve a textual flags change
>  (disregarding any change in behavior), or to actually change toolchain
>  behavior?
>  
The goal is to make it easy to remove individual flags so packages can
get the toolchain behavior they want.

>  If the former, I'm not sure if the actual _flag names are useful.
>  Maybe
>  we can add an RPM macro to suppress or replace flags based on the flags
>  as they are used instead.  This could also add additional error checking
>  because a name typo in the macro definition will not be immediately
>  obvious.
>   
Can you give an example of what you mean by this?

> > With these new macros, the examples from above could be
> re-written as:
> >
> >      compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
> >      julia:       %global _flag_glibcxx_assertions %{nil}
>  
>  Do you have some background why -D_DEFAULT_SOURCE is needed?  Why
>  doesn't upstream detect this?
>   
It was added to workaround this bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=25271

-Tom

>  Thanks,
>  Florian
>  _______________________________________________
>  devel mailing list -- devel(a)lists.fedoraproject.org
>  To unsubscribe send an email to devel-leave(a)lists.fedoraproject.org
>  Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
>  List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
>  List Archives:
> https://lists.fedoraproject.org/archives/list/[e-mail]
>  Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure

[Florian Weimer]

* Tom Stellard:

>  On 6/7/22 02:18, Florian Weimer wrote:
> > * Ben Cotton:
> > 
> >> This change will add new macros which will make it easier for packages
> >> to add and remove their own compiler flags.  This strategy is already
> >> used to some extent with feature macros like %{_lto_cflags},
> >> %{_hardening_cflags}, etc, but these new macros will give packagers
> >> even more fine-grained control over the options.
> >>
> >> The proposed macros for adding new flags are:
> >>
> >>      %_pkg_extra_cflags
> >>      %_pkg_extra_cxxflags
> >>      %_pkg_extra_fflags
> >>      %_pkg_extra_ldflags
> > Why isn't it possible to use the environment variables for this?
> > 
>
>  Environment variables won't cover all cases.  For example, some packages
>  need the flags passed as arguments to make or configure or some other
>  build system. 
But then you can stick the extra flags into that argument.  I don't see
why this additional hook is needed.

> > This is very misleading because in several cases, clearing the
> those
> > flags will not affect toolchain behavior because the flag in question
> > merely restates the toolchain default.  In particular, this applies to
> > -fasynchronous-unwind-tables, and it would apply to -march= and -mcpu=
> > if those were in the list.  Likewise to -Wl,-z,relro.
> > Is the goal of this proposal just to achieve a textual flags change
> > (disregarding any change in behavior), or to actually change toolchain 
>  The goal is to make it easy to remove individual flags so packages
> can
>  get the toolchain behavior they want. 
Sorry, this does not answer my question.  What if removing the flag does
not actually change toolchain behavior?

> > If the former, I'm not sure if the actual _flag names are
> useful.  Maybe
> > we can add an RPM macro to suppress or replace flags based on the flags
> > as they are used instead.  This could also add additional error checking
> > because a name typo in the macro definition will not be immediately
> > obvious.
> > 
>
>  Can you give an example of what you mean by this? 
You could write:

%build_cxxflags_remove -Wp,-D_GLIBCXX_ASSERTIONS

or some similar construct, and -Wp,-D_GLIBCXX_ASSERTIONS would be
removed from the build flags.

> >> With these new macros, the examples from above could be
> re-written as:
> >>
> >>      compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
> >>      julia:       %global _flag_glibcxx_assertions %{nil}
> > Do you have some background why -D_DEFAULT_SOURCE is needed?  Why
> > doesn't upstream detect this?
> > 
>
>  It was added to workaround this bug:
> https://sourceware.org/bugzilla/show_bug.cgi?id=25271 
This has long since been fixed, so the workaround is no longer needed.

-D_GLIBCXX_ASSERTIONS has zero false positives.  Removing it does not
make the code that triggers the asserts well-defined.  Only the explicit
assertion failure is gone.  So it is probably another example of a flag
where removal does not result in the hoped-for change in toolchain
behavior.

Thanks,
Florian

[Tom Stellard]

On 6/24/22 01:32, Florian Weimer wrote:
>  * Tom Stellard:
>  
> > On 6/7/22 02:18, Florian Weimer wrote:
> >> * Ben Cotton:
> >>
> >>> This change will add new macros which will make it easier for packages
> >>> to add and remove their own compiler flags.  This strategy is already
> >>> used to some extent with feature macros like %{_lto_cflags},
> >>> %{_hardening_cflags}, etc, but these new macros will give packagers
> >>> even more fine-grained control over the options.
> >>>
> >>> The proposed macros for adding new flags are:
> >>>
> >>>       %_pkg_extra_cflags
> >>>       %_pkg_extra_cxxflags
> >>>       %_pkg_extra_fflags
> >>>       %_pkg_extra_ldflags
> >> Why isn't it possible to use the environment variables for this?
> >>
> >
> > Environment variables won't cover all cases.  For example, some packages
> > need the flags passed as arguments to make or configure or some other
> > build system.
>  
>  But then you can stick the extra flags into that argument.  I don't see
>  why this additional hook is needed.
>  
> >> This is very misleading because in several cases, clearing the those
> >> flags will not affect toolchain behavior because the flag in question
> >> merely restates the toolchain default.  In particular, this applies to
> >> -fasynchronous-unwind-tables, and it would apply to -march= and -mcpu=
> >> if those were in the list.  Likewise to -Wl,-z,relro.
> >> Is the goal of this proposal just to achieve a textual flags change
> >> (disregarding any change in behavior), or to actually change toolchain
>  
> > The goal is to make it easy to remove individual flags so packages can
> > get the toolchain behavior they want.
>  
>  Sorry, this does not answer my question.  What if removing the flag does
>  not actually change toolchain behavior?
>   
This was not something I had considered.  We could update the macro so
that if a macro was set to %{nil} then the flag to turn off the feature
is used so something like:

%build_cflags %[ "%{_flag_asynchronous_unwind_tables}" == "" ?
"-fno-asynchronous-unwind-tables" :
"%{_flag_asynchronous_unwind_tables}"] ...


Or we could just leave it as is and document that the default complier
behavior takes precedence over compiler flags.  The proposed change doesn't actually
make the problem worse, because the current method for removing compiler flags
has the same problem: e.g.

%global optflags %(echo %{optflags} | sed 's/-fasynchronous-unwind-tables//

will remove the flag but not change the behavior.

- Tom

> >> If the former, I'm not sure if the actual _flag names are
> useful.  Maybe
> >> we can add an RPM macro to suppress or replace flags based on the flags
> >> as they are used instead.  This could also add additional error checking
> >> because a name typo in the macro definition will not be immediately
> >> obvious.
> >>
> >
> > Can you give an example of what you mean by this?
>  
>  You could write:
>  
>  %build_cxxflags_remove -Wp,-D_GLIBCXX_ASSERTIONS
>  
>  or some similar construct, and -Wp,-D_GLIBCXX_ASSERTIONS would be
>  removed from the build flags.
>  
> >>> With these new macros, the examples from above could be re-written as:
> >>>
> >>>       compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
> >>>       julia:       %global _flag_glibcxx_assertions %{nil}
> >> Do you have some background why -D_DEFAULT_SOURCE is needed?  Why
> >> doesn't upstream detect this?
> >>
> >
> > It was added to workaround this bug:
> https://sourceware.org/bugzilla/show_bug.cgi?id=25271
>  
>  This has long since been fixed, so the workaround is no longer needed.
>  
>  -D_GLIBCXX_ASSERTIONS has zero false positives.  Removing it does not
>  make the code that triggers the asserts well-defined.  Only the explicit
>  assertion failure is gone.  So it is probably another example of a flag
>  where removal does not result in the hoped-for change in toolchain
>  behavior.
>  
>  Thanks,
>  Florian
>

[Florian Weimer]

* Tom Stellard:

>  Or we could just leave it as is and document that the default
> complier
>  behavior takes precedence over compiler flags.  The proposed change
>  doesn't actually make the problem worse, because the current method
>  for removing compiler flags has the same problem: e.g.
>
>  %global optflags %(echo %{optflags} | sed 's/-fasynchronous-unwind-tables//
>
>  will remove the flag but not change the behavior. 
Independently of that, I think we should still provide a facility that
is more similar to the sed hack (perhaps with additional error
checking), rather than maintain a set of new variables.

Thanks,
Florian

[Jonathan Wakely]

On Fri, 24 Jun 2022 at 09:32, Florian Weimer wrote:
>  >>> With these new macros, the examples from above could be
> re-written as:
>  >>>
>  >>>      compiler-rt: %global _pkg_extra_cflags -D_DEFAULT_SOURCE
>  >>>      julia:       %global _flag_glibcxx_assertions %{nil}
>  >> Do you have some background why -D_DEFAULT_SOURCE is needed?  Why
>  >> doesn't upstream detect this?
>  >>
>  >
>  > It was added to workaround this bug:
> https://sourceware.org/bugzilla/show_bug.cgi?id=25271
>
>  This has long since been fixed, so the workaround is no longer needed.
>
>  -D_GLIBCXX_ASSERTIONS has zero false positives.  Removing it does not
>  make the code that triggers the asserts well-defined.  Only the explicit
>  assertion failure is gone.  So it is probably another example of a flag
>  where removal does not result in the hoped-for change in toolchain
>  behavior. 
Yes, I don't think an example of how to hide undefined behaviour is a
good example for the change proposal.

[Florian Weimer]

I want to add documentation for this change, and found this proposal
self-contradictory.

>  == Summary ==
>  Create a corresponding macro for each compiler flag in the
>  redhat-rpm-config macro file and create "extra flag" macros to make it
>  easier for packages to add and remove compiler flags. 
>  == Benefit to Fedora ==
>  * It will provide a standard way to disable existing compiler flags or
>  enable new ones that is more simple and robust than the existing echo
>  + sed solution. 
(I believe this is no longer relevant to the second version of the
proposal.)

>  * It will make it easier to determine which packages disable or add
>  compiler flags by doing a simple grep of the spec files.
>  * It will make it easier for toolchain developers to experiment with
>  adding new flags to the distribution as this can be done with a simple
>  macro definition instead of patching redhat-rpm-config. 
The main problem are these two goals.  I don't think you can have it
both ways.

Here's a concrete example.  The proposal mentions compiler-rt.spec as a
potential use case.  Say it changes

    %global optflags %(echo %{optflags} -D_DEFAULT_SOURCE)

to:

    %global _pkg_extra_cflags -D_DEFAULT_SOURCE

While this is arguably useful (second use case), after this change, it
is no longer possible to inject additional build flags via
_pkg_extra_cflags from the outside (third use case).

Do we actually need two sets of macros, with distinct use cases?

Thanks,
Florian

[Tom Stellard]

On 2/10/23 00:07, Florian Weimer wrote:
>  I want to add documentation for this change, and found this proposal
>  self-contradictory.
>  
> > == Summary ==
> > Create a corresponding macro for each compiler flag in the
> > redhat-rpm-config macro file and create "extra flag" macros to make it
> > easier for packages to add and remove compiler flags.
>  
> > == Benefit to Fedora ==
> > * It will provide a standard way to disable existing compiler flags or
> > enable new ones that is more simple and robust than the existing echo
> > + sed solution.
>  
>  (I believe this is no longer relevant to the second version of the
>  proposal.)
>  
> > * It will make it easier to determine which packages disable or add
> > compiler flags by doing a simple grep of the spec files.
> > * It will make it easier for toolchain developers to experiment with
> > adding new flags to the distribution as this can be done with a simple
> > macro definition instead of patching redhat-rpm-config.
>  
>  The main problem are these two goals.  I don't think you can have it
>  both ways.
>  
>  Here's a concrete example.  The proposal mentions compiler-rt.spec as a
>  potential use case.  Say it changes
>  
>       %global optflags %(echo %{optflags} -D_DEFAULT_SOURCE)
>  
>  to:
>  
>       %global _pkg_extra_cflags -D_DEFAULT_SOURCE
>  
>  While this is arguably useful (second use case), after this change, it
>  is no longer possible to inject additional build flags via
>  _pkg_extra_cflags from the outside (third use case).
>  
>  Do we actually need two sets of macros, with distinct use cases?
>   
This is a good point.  So, it seems like adding something like
%_distro_extra_cflags in addition to %_pkg_extra_cflags might make
sense.  However, as I've been working on this, it seems to me that
the ability to inject distro-wide build flags (%_distro_extra_flags)
is much more useful that having a convenience macro for packages to
add their own flags (%_pkg_extra_cflags).  So, just renaming %_pkg_extra_flags
to %_distro_extra_cflags would be OK with me too.


-Tom


>  Thanks,
>  Florian
>

[Florian Weimer]

* Tom Stellard:

> >> * It will make it easier to determine which packages disable
> or add
> >> compiler flags by doing a simple grep of the spec files.
> >> * It will make it easier for toolchain developers to experiment with
> >> adding new flags to the distribution as this can be done with a simple
> >> macro definition instead of patching redhat-rpm-config.
> > The main problem are these two goals.  I don't think you can have it
> > both ways.
> > Here's a concrete example.  The proposal mentions compiler-rt.spec
> > as a
> > potential use case.  Say it changes
> >      %global optflags %(echo %{optflags} -D_DEFAULT_SOURCE)
> > to:
> >      %global _pkg_extra_cflags -D_DEFAULT_SOURCE
> > While this is arguably useful (second use case), after this change,
> > it
> > is no longer possible to inject additional build flags via
> > _pkg_extra_cflags from the outside (third use case).
> > Do we actually need two sets of macros, with distinct use cases?
> > 
>
>  This is a good point.  So, it seems like adding something like
>  %_distro_extra_cflags in addition to %_pkg_extra_cflags might make
>  sense.  However, as I've been working on this, it seems to me that
>  the ability to inject distro-wide build flags (%_distro_extra_flags)
>  is much more useful that having a convenience macro for packages to
>  add their own flags (%_pkg_extra_cflags).  So, just renaming %_pkg_extra_flags
>  to %_distro_extra_cflags would be OK with me too. 
Yes, that sounds reasonable.  We can mention these macros in
buildflags.md and advise packagers not to use them or change them.
The _distro_ prefix aligns with that as well.

Thanks,
Florian