Testing:

$ rpm --macros=data/macros.rust -E '%build_rustflags'
-Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Clink-arg=-Wl,-z,relro -Clink-arg=-Wl,-z,now --cap-lints=warn
$ rpm --macros=data/macros.rust -D '%_include_frame_pointers 1' -E '%build_rustflags'
-Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Cforce-frame-pointers=yes -Clink-arg=-Wl,-z,relro -Clink-arg=-Wl,-z,now --cap-lints=warn

:thumbsup:

I would have appreciated it if you had at least waited until I could take a look at this. I don't like the way this is implemented right now, as it doesn't match the other rpm macros that can be used to control compiler flags. :imp:

That's how package notes were done, so I considered it reasonable to implement in a similar fashion.

The package notes Implementation doesn't match what's currently in Fedora, the recent changes required a downstream patch that's still pending in a PR because zbyszek seems to direct emails to /dev/null.

Also, package notes affects linker flags, not rustc / compiler flags, so it's not exactly a fair comparison.

After taking another look at this that wasn't through my phone screen, the changes in this PR look good to me. The flag for controlling the inclusion of -Cforce-frame-pointers=yes is not specific to Rust, and indeed should probably only be set globally for all C-like programming languages, and opting out Rust while it's enabled for C code from the same build would be weird.

I've backported the patch to the Fedora package and triggered a build. Just to make sure the Rust stack is ready for the mass rebuild (assuming the %_include_frame_pointer 1 default is going to be set by then).

https://koji.fedoraproject.org/koji/buildinfo?buildID=2107228

Hey @dcavalca, is it possible that this change is partly broken? Looking at build logs from the mass rebuild, it appears that builds on all architectures are done with -Cforce-frame-pointers=yes for some reason, even though it should be disabled there.

Hmm, maybe we should change it to %{expr:0%{?_include_frame_pointers} ? "-Cforce-frame-pointers=yes" : ""}?

Oh wait, I know why it's happening, it's because redhat-rpm-config isn't conditionally enabling the frame pointer variable. It's enabled on all arches, and then it's selectively ignored on some architectures.

Can this be fixed in redhat-rpm-macros, please? I'd rather not have to keep the list of architectures where they are or are not enabled in sync here.

It's only disabled on i686 and s390x for now for C/C++. Are we seeing failures beyond that?

I'm not sure about failures, but I don't think it's a good idea to build Rust programs with frame pointers enabled across all architectures if they have caused problems on i686, ppc64le, and s390x. Checking recent build logs, the -Cforce-frame-pointers=yes flag is now included everywhere.

I'm not sure if C/C++ issues imply Rust issues, though.

Sorry folks, this is my fault. What happened is that when I wrote this the redhat-rpm-config was using %ifarch to conditionally define the macro only on the architectures where it should be enabled. I later discovered that didn't work and changed the approach, by only including the flags in rpmrc on the architectures where this is enabled (akin to how other flag-based macros are handled there), but forgot to update this accordingly.

Unfortunately I don't see a way to gate macro enablement by architecture in redhat-rpm-config, so we might have to keep the arch list here as well (definitely open to suggestions if folks have ideas though).

From a quick look over https://kojipkgs.fedoraproject.org/mass-rebuild/f38-failures.html I don't see this causing a significant amount of failures in rust packages. Nonetheless, enabling this everywhere for Rust is a deviation from the rest of the distribution and from the Change as it was approved, so I think we should fix it. I tried putting up a PR to match the gating in rust2rpm, but it looks like %ifarch doesn't work here either, so I'm not entirely sure how to go about it.

If it's not possible to define something like %frame_pointer_arches in redhat-rpm-config, I'm fine with duplicating the list of architectures in the Rust macros for now (that would be x86_64 and aarch64 now, right?) ... I'll probably do a mini-mass-rebuild of Rust applications once that's fixed. I've seen at least one build failure that was probably caused by frame pointers on i686, and I've only looked at failures of my own packages so far.

Fixup for this in https://pagure.io/fedora-rust/rust2rpm/pull-request/239