Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1525568 - redhat-rpm-config: Split out environment setup from %configure
Summary: redhat-rpm-config: Split out environment setup from %configure
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: redhat-rpm-config
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Florian Weimer
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1535422
TreeView+ depends on / blocked
 
Reported: 2017-12-13 15:14 UTC by Florian Weimer
Modified: 2018-02-04 17:22 UTC (History)
10 users (show)

Fixed In Version: redhat-rpm-config-94-1.fc28
Doc Type: Enhancement
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-02-04 17:22:49 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Patch to introduce %build_* macros (3.11 KB, patch)
2018-01-17 10:24 UTC, Florian Weimer
no flags Details | Diff
Patch to introduce %build_* macros (5.70 KB, patch)
2018-01-26 12:43 UTC, Florian Weimer
no flags Details | Diff

Description Florian Weimer 2017-12-13 15:14:18 UTC
Not every package uses autoconf, and %configure is currently the only way to set CFLAGS, CXXFLAGS, and so on.

Various packagers have resorted to different means to use use %configure without autoconf, to set the environment as required.  It would be preferable if there was one standard, documented way to do this, and which does not rely on internals of the %configure implementation.

Comment 2 Panu Matilainen 2017-12-14 07:55:38 UTC
Hmm, I would've sworn I already did just that, but clearly not. Another memory cell malfunction, hmph. But yes, absolutely.

Comment 3 Florian Weimer 2018-01-17 10:24:47 UTC
Created attachment 1382329 [details]
Patch to introduce %build_* macros

I'm attaching a patch to introduce various %build_* macros and some comments to document them.

I tested this by building bash, lua, ninja-build.

I'm keeping the old %__global_* names for now because some existing spec files use them.

Comment 4 Panu Matilainen 2018-01-17 11:45:37 UTC
%__global_compiler_flags is used in rpmrc too, that needs to be updated as per the new name if the old one is deprecated.

Other than that, I don't see anything obviously wrong. As with any naming related thing, there's endless potential for bikeshedding, such as:
- why not just %cflags, %ldflags etc
- ...or %CFLAGS, %LDFLAGS etc (but might actually be too easy to mixup with the shell variables)
...but I've no particular problem with the %build_foo naming either.

Let me mull over this a bit. I'm actually looking/planning to upstream the result of this into rpm itself eventually so if something I say here sounds out of context... but don't let me get hung over that.

On a related note, %__global_ldflags is getting also passed to the build environment as RPM_LD_FLAGS in Fedora (which is also been to-be-upstreamed-some-day-in-some-form since forever):
https://src.fedoraproject.org/rpms/rpm/c/4dd6dd15e611bac9d48893bdd9ed0cfb0e978818?branch=master

Comment 5 Florian Weimer 2018-01-17 12:03:11 UTC
I deliberately did not touch %__global_compiler_flags in my patch.  I'll add a reference to RPM_LD_FLAGS to an updated patch.

Comment 6 Panu Matilainen 2018-01-17 12:44:48 UTC
Oops, sorry. Not looking carefully enough, obviously.

Comment 7 Panu Matilainen 2018-01-18 14:20:36 UTC
Actually the bigger reason for mentioning RPM_LD_FLAGS was that it points out another (alternative) way of exporting this data to the environment. We could just as well export the existing __global_* flags via RPM_FOO_FLAGS env variables, only in that case the patch goes to rpm and not redhat-rpm-config. Or do both...

Comment 8 Florian Weimer 2018-01-18 14:30:08 UTC
I would like to concentrate most of the flag maintenance in redhat-rpm-config.  It may also be desirable to add a Lua-based macro which allows queries like “give me all warning flags” or “give me all code generation/ABI variant flags”.

Comment 9 Panu Matilainen 2018-01-19 08:53:54 UTC
Sure, and exporting (additionally) via environment variables doesn't actually change that. The flags are set in redhat-rpm-config macros, only the export part needs to go to rpm because we're not overriding %___build_pre. That's how the ldflags-thing is done atm.

Comment 10 Florian Weimer 2018-01-26 12:43:40 UTC
Created attachment 1386529 [details]
Patch to introduce %build_* macros

Updated patch, now with documentation in buildflags.md.

I did not make any material changes, the way things are structured is essentially the same.  I do not think it is worth the effort to try to redistribute the flag setting responsibilities between rpm/%build and this package.

Comment 11 Panu Matilainen 2018-02-02 09:51:58 UTC
I'm coming down with a flu and not feeling particularly well or bright today, but lets try to get this thing moving... At least one concrete bug introduced in the patch:

+* `%{build_fclags} for `FFLAGS` (the Fortran compiler flags, also
+  known as the `FCFLAGS` variable).
[...]
+  FFLAGS="${FFLAGS:-%{build_fflags}}" ; export FFLAGS ; \
+  FCFLAGS="${FCFLAGS:-%{build_fcflags}}" ; export FCFLAGS ; \

These refer to %{build_fcflags} but that's not defined in the patch, only %{build_fflags} is.

+%__global_fflags %{build_fflags}
+%__global_fcflags %{build_fflags}

...and then the latter one should be %{build_fcflags}.

[...]
+# Note: __global_ldflags is used by RPM in %%___build_pre to set RPM_LD_FLAGS.

I'd leave that implementation detail out, we can just switch rpm to use the new value once its in redhat-rpm-config (and perhaps export the other values similarly in the environment while at it).

I've had couple of weeks to mull over it and nothing's come out of it so ... after fixing the fflags/fcflags confusion (and maybe the ldflags thing) just go ahead with.

Comment 12 Florian Weimer 2018-02-04 17:22:49 UTC
Thanks, applied with these fixes.


Note You need to log in before you can comment on or make changes to this bug.