-
Notifications
You must be signed in to change notification settings - Fork 113
Issue 7178 - Bundled jemalloc fails to build with GCC 15 #7216
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- The
srpmstarget now hardcodes copying a single patch into$(RPMBUILD)/SOURCES; consider generalizing this (e.g., copying allrpm/*.patchfiles or using a variable for patch names) so future jemalloc or related patches don't require additional makefile edits.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `srpms` target now hardcodes copying a single patch into `$(RPMBUILD)/SOURCES`; consider generalizing this (e.g., copying all `rpm/*.patch` files or using a variable for patch names) so future jemalloc or related patches don't require additional makefile edits.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Description: Update spec file to fix build failures on Fedora Rawhide Fixes: 389ds#7178
progier389
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM:
As @fiirstyear mentionned, we should start to think about jemalloc replacement but in the mean time this commit will fix the Fedora RawHide build break
droideck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor issues, otherwise, looks good!
|
|
||
| # The following are needed to build the snmp ldap-agent | ||
| BuildRequires: net-snmp-devel | ||
| # libnl3 hould be a transient dependency from net-snmp-devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # libnl3 hould be a transient dependency from net-snmp-devel | |
| # libnl3 should be a transient dependency from net-snmp-devel |
Also, should it be transitive dependency? As the one that comes indirectly through another package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both :D
It was needed temporarily, but it's fixed now https://src.fedoraproject.org/rpms/net-snmp/pull-request/13
| BuildRequires: npm | ||
| BuildRequires: nodejs | ||
| BuildRequires: /usr/bin/npm | ||
| BuildRequires: /usr/bin/node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion here but it's unrelated changed and it, probably, should be in a separate PR (cleaner backports).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR talks about failures on Rawhide, this is one of them caused by https://fedoraproject.org/wiki/Changes/NodejsAlternativesSystem
| @@ -188,9 +190,7 @@ Requires: %{name}-robdb-libs = %{version}-%{release} | |||
| Requires: libdb | |||
| %endif | |||
| %endif | |||
| Requires: lmdb | |||
| # This picks up libperl.so as a Requires, so we add this versioned one | |||
| Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version)) | |||
| Requires: lmdb-libs | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Unrelated change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, but I wanted to clean this up since it's fixed in Rawhide.
| fi | ||
|
|
||
| srpms: rpmroot srpmdistdir download-cargo-dependencies tarballs rpmbuildprep | ||
| cp rpm/jemalloc-5.3.0_throw_bad_alloc.patch $(RPMBUILD)/SOURCES/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to copy the source files during rpmbuildprep target? Other source files copies happen there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, I'll move it there, thanks!
Description:
Update spec file to fix build failures on Fedora Rawhide
Fixes: #7178
Summary by Sourcery
Build: