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.

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

app-admin/cronolog string.h patch Closes #894196 #33775

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mipseb
Copy link
Contributor

@mipseb mipseb commented Nov 12, 2023

patches app/admin/cronolog to include <string.h> in cronolog.c, without which an error for an undeclared library function will occur. This fix does not currently work on musl libc systems.

@gentoo-bot gentoo-bot added maintainer-needed There is at least one affected package with no maintainer. Review it if you can. assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Nov 12, 2023
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-11-12 00:36 UTC
Newest commit scanned: 0502bf3
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/20a77807c2/output.html

@mipseb mipseb closed this Nov 12, 2023
@mipseb mipseb reopened this Nov 12, 2023
@mipseb mipseb marked this pull request as ready for review November 12, 2023 05:15
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-11-12 05:26 UTC
Newest commit scanned: f263c25
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/a876aa0573/output.html

@mipseb mipseb changed the title app-admin/cronolog string.h patch Closes #894196 app-admin/cronolog string.h patch Closes #894196 [please reassign] Nov 12, 2023
@gentoo-bot gentoo-bot changed the title app-admin/cronolog string.h patch Closes #894196 [please reassign] app-admin/cronolog string.h patch Closes #894196 Nov 12, 2023
@gentoo-bot
Copy link

Pull Request assignment

Submitter: @mipseb
Areas affected: ebuilds
Packages affected: app-admin/cronolog

app-admin/cronolog: @gentoo/proxy-maint (maintainer needed)

Linked bugs

Bugs linked: 894196


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added maintainer-needed There is at least one affected package with no maintainer. Review it if you can. assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. and removed assigned PR successfully assigned to the package maintainer(s). maintainer-needed There is at least one affected package with no maintainer. Review it if you can. no bug found No Bug/Closes found in the commits. labels Nov 12, 2023
@Kangie
Copy link
Contributor

Kangie commented Nov 13, 2023

Please squash your commits; that's only one change. Try a title like app-admin/cronolog: fix implicit-function-declaration for the combined commit.

Thanks for linking the relevant bug.

Unfortunately I'm not sure this works, in a musl llvm chroot I'm still seeing build failures due to implicit-function-declarations:

getopt.c:577:30: error: call to undeclared library function 'strcmp' with type 'int (const char *, const char *)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
      if (optind != argc && !strcmp (argv[optind], "--"))
                             ^
getopt.c:577:30: note: include the header <string.h> or explicitly provide a declaration for 'strcmp'
getopt.c:653:7: error: call to undeclared library function 'strncmp' with type 'int (const char *, const char *, unsigned long)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
        if (!strncmp (p->name, nextchar, nameend - nextchar))
             ^
getopt.c:653:7: note: include the header <string.h> or explicitly provide a declaration for 'strncmp'
getopt.c:656:21: error: call to undeclared library function 'strlen' with type 'unsigned long (const char *)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                == (unsigned int) strlen (p->name))
                                  ^
getopt.c:656:21: note: include the header <string.h> or explicitly provide a declaration for 'strlen'

Did you validate that this builds on musl? Or is that what

my issue is actually present specifically on LLVM/Musl systems

refers to?

Regardless, the fix is what you've already done.

I'm not sure that we should keep this package on life support after this tbh. It's been about 20 years since anyone cared to properly update it and we have an absurd number of patches to maintain.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-11-14 00:37 UTC
Newest commit scanned: 504b96c
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/d8fec2c431/output.html

@Kangie
Copy link
Contributor

Kangie commented Nov 14, 2023

Germ and I looked at this a bit more closely (and thanks to Sam for rubber-duckying):

  • The SRC_URI is dead and has been for a long time
  • Cronolog isn't properly detecting defining something relating to string.h on musl libc - probably because config.h is very basic.
  • The upstream copy on GitHub is v1.6.3 and includes a proper config.h.in among other fixes, which should "just work"

Path forward:

As the number of patches is approaching insanity (9 currently), we will fork the upstream repo, rebase and apply our existing patches (I expect this to be mostly clean), bump the ebuild to 1.6.3_p, test, etc.

…rncrpy, leading to an

call to undeclared library.  closes #894196
Signed-off-by: Germ Mipseb <germtoo@outlook.com>
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2023-11-15 06:02 UTC
Newest commit scanned: 432a10f
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/27827487db/output.html

@dlan17
Copy link
Contributor

dlan17 commented Jan 7, 2024

Path forward:

As the number of patches is approaching insanity (9 currently), we will fork the upstream repo, rebase and apply our existing patches (I expect this to be mostly clean), bump the ebuild to 1.6.3_p, test, etc.

sounds a good plan.. any progress? ^_^

@Kangie
Copy link
Contributor

Kangie commented Jan 7, 2024

sounds a good plan.. any progress? ^_^

I got side tracked. Currently on disaster relief stuff for a little bit; might be a month or so while I mentor germ on it. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. maintainer-needed There is at least one affected package with no maintainer. Review it if you can.
Projects
None yet
5 participants