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

media-libs/sdl-net: EAPI8 bump, minor ebuild improvements #21772

Closed
wants to merge 1 commit into from

Conversation

mm1ke
Copy link
Contributor

@mm1ke mm1ke commented Jul 24, 2021

Package-Manager: Portage-3.0.20, Repoman-3.0.3
Signed-off-by: Michael Mair-Keimberger mmk@levelnine.at

I've looked into https://bugs.gentoo.org/730872 too and i think the problem was because it called AS directly. Please have a look. In the config.status file it now uses the correct tool, but i couldn't see anywhere in the logs that it was used.

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @mm1ke
Areas affected: ebuilds
Packages affected: media-libs/sdl-net

media-libs/sdl-net: @gentoo/games

Linked bugs

Bugs linked: 730872


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 assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Jul 24, 2021
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-24 20:24 UTC
Newest commit scanned: 848e811
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/7d73d89071/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-24 22:54 UTC
Newest commit scanned: 7931f16
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/d00406648f/output.html

DEPEND="${RDEPEND}"

multilib_src_configure() {
AS="$(tc-getAS)" ECONF_SOURCE="${S}" econf \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you been able to reproduce? It's best not do guessed fixes, can't tell if it really fixed anything.

I tried -native-symlinks (i.e. no "as" command), and I still don't get this. exporting empty AS="" doesn't affect this either so it's unlikely setting this affect anything.

There's also more to it, the tinderbox bug didn't clearly mention it but it used llvm toolchain:

checking for x86_64-pc-linux-gnu-gcc... x86_64-pc-linux-gnu-clang
checking for ld used by x86_64-pc-linux-gnu-clang... ld.lld
ecking for BSD- or MS-compatible name lister (nm)... llvm-nm
checking for x86_64-pc-linux-gnu-objdump... llvm-objdump
checking for x86_64-pc-linux-gnu-ar... llvm-ar
checking for x86_64-pc-linux-gnu-strip... llvm-strip
checking for x86_64-pc-linux-gnu-ranlib... llvm-ranlib
checking how to run the C preprocessor... x86_64-pc-linux-gnu-clang -E

I tried with llvm/clang as well but still nothing. Either this got indirectly fixed (possible considering line 46 of current ./libtool can't fail like this) or there's something up with how the tinderbox setup the llvm toolchain back then.

I'm likely going to close that bug as WORKSFORME instead and check it again only if it resurfaces.

So, you can remove this and the bug reference unless did reproduce.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Un)fortunately i couldn't reproduce it either.

What i found out was that after configuring the package, the generated libtool script contains a line with as=AS. However, i couldn't find anywhere in the build that AS was used actually.
I also saw that the llvm toolchain was used and was thinking this bug happens in combination with llvm (thus my initial note that i think i fixed it)..
And with setting AS="$(tc-getAS)" i could fix the libtool file to set it to the correct system variable..

However, since you also couldn't reproduce the problem with the llvm toolchain, I'm happily remove the workaround - and sorry that i haven't explained it more in detail.

multilib_src_configure() {
AS="$(tc-getAS)" ECONF_SOURCE="${S}" econf \
--disable-gui \
$(use_enable static-libs static)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this and IUSE="static-libs", I see no reason to support static-libs anymore on this.

EAPI-8's econf will pass --disable-static by default

Since will be only 1 option left, econf call should fit nicely on a single line without \

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i've also removed it now from the smpeg{,2} ebuilds in the other PR. I've checked the reverse-dependencies and it's not used anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, indeed I don't see a need for it on smpeg either (hadn't looked yet).

All these libraries tend to need more attention and if going to revbump into ~arch want to get more done at once, part of why nobody bothered to touch them yet -- but this had to be done sooner or later.

Comment on lines 8 to 10
MY_P="${P/sdl-/SDL_}"
DESCRIPTION="Simple Direct Media Layer Network Support Library"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer a clear cut for the description/homepage block, so I usually have a blank line between these.

Comment on lines 30 to 32
if ! use static-libs ; then
find "${D}" -type f -name '*.la' -exec rm {} + || die
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make deletion unconditional but use the find statement from the policy guide:
https://projects.gentoo.org/qa/policy-guide/installed-files.html#pg0303

}

multilib_src_install_all() {
dodoc CHANGES README
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES and README are part of einstalldocs' defaults, so should just use that.


MY_P="${P/sdl-/SDL_}"
DESCRIPTION="Simple Direct Media Layer Network Support Library"
HOMEPAGE="https://www.libsdl.org/projects/SDL_net/index.html"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem can drop the index.html bit.

@mm1ke mm1ke force-pushed the sdln-lmalFnnA branch 2 times, most recently from 90e409d to 31254b8 Compare July 28, 2021 17:31
Package-Manager: Portage-3.0.20, Repoman-3.0.3
Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
Bug: https://bugs.gentoo.org/730872
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-28 17:44 UTC
Newest commit scanned: 31254b8
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/3532606ffc/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-28 18:19 UTC
Newest commit scanned: 110bbaa
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/53163c56dd/output.html

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.
Projects
None yet
4 participants