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

check-reqs.eclass: clamp MAKEOPTS based on available memory #23311

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

Conversation

thesamesam
Copy link
Member

@thesamesam thesamesam commented Dec 15, 2021

Kept qtwebengine changes from #23310 in this branch to allow easy testing. Please ignore that for purposes of review.

Very rough and not tested much yet.

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @thesamesam
Areas affected: ebuilds, eclasses
Packages affected: dev-qt/qtwebengine

dev-qt/qtwebengine: @gentoo/qt, @gyakovlev

Linked bugs

Bugs linked: 570534

Missing GCO sign-off

Please read the terms of Gentoo Certificate of Origin and acknowledge them by adding a sign-off to all your commits.


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. no signoff One or more commits do not indicate GCO sign-off. labels Dec 15, 2021
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-12-15 05:35 UTC
Newest commit scanned: 36e30ed
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/355673edc8/output.html

@mgorny
Copy link
Member

mgorny commented Dec 15, 2021

To be honest, I'm not sure if this is conceptually sound.

The njobs * 2 GiB requirement is the absolute minimum we expect from users and we don't check for that. If a package uses check-reqs, then it means it has requirements above the minimum. In that case, clipping jobs to match our minimum requirement doesn't resolve the problem.

I mean, you're adding a workaround for misconfigured systems to packages where normally it isn't supposed to help anyway.

@thesamesam
Copy link
Member Author

thesamesam commented Dec 15, 2021

To be honest, I'm not sure if this is conceptually sound.

The njobs * 2 GiB requirement is the absolute minimum we expect from users and we don't check for that. If a package uses check-reqs, then it means it has requirements above the minimum. In that case, clipping jobs to match our minimum requirement doesn't resolve the problem.

I mean, you're adding a workaround for misconfigured systems to packages where normally it isn't supposed to help anyway.

I take your point (and agree it is the absolute minimum) but it doesn't change reality either. We get a lot of users who don't realise that's the minimum. The handbook now documents reasonable standards but that doesn't help the thousands and thousands of users over the years who never updated it (and tbh, why should they know to update it?).

My expectation would be we use this for pkgs that:

  1. has significant memory use and;
  2. takes a while to build would use check-reqs for the memory component to save users some pain.

Aside: I don't actually think we're even using check-reqs in its current tree state in enough packages anyway for stuff above the minimum requirements.

Not that many fit into both: qtwebengine, maybe llvm/clang, firefox, spidermonkey, rust, webkit-gtk, chromium. So, I don't expect to add this everywhere I just want to have it to avoid having to continue marking qtwebengine dupes and avoid frustration when a long-running build fails. I don't think too much falls into that bin.

Alternatively:

  • we warn in Portage? I don't think we could get away with clamping there though, it would have to be just a warning, which isn't very satisfactory.
  • we make Portage grep for classic OOM-style errors in build.log and warn about it accordingly? (I've wanted to do this for some Perl-style stuff too)

Any other ideas? My ultimate goal is avoiding OOM bugs (and ideally users being frustrated by failed long builds).

(I also have sympathy for people who try to have a higher njobs but don't realise what the big packages are. It kind of sucks if you're on a box with a lot of cores that is slow at single-core perf. Most stuff will build fine with way higher than RAM/2GB. But a few won't.)

@mgorny
Copy link
Member

mgorny commented Dec 15, 2021

A warning in Portage would be a good first step. Then we can see how often the problem happens still.

@mattst88
Copy link
Contributor

I've never found the 2G*proc limit to be critical for anything except the largest packages (like firefox, chromium, etc). Those packages are precisely the ones that use the check-reqs eclass.

@thesamesam
Copy link
Member Author

thesamesam commented Dec 17, 2021

Upon reflection (and after spending a lot of time looking at Portage earlier for something else), I don't really want to add even more complexity to Portage if I can help it for now.

As probably one of the main devs who handles user support, OOMs happen all the time and part of the reason is that -jN where N = cores without regard for RAM actually works until you hit a big boy like chromium, hence they're confused when they find it doesn't work on a handful of packages.

@thesamesam thesamesam force-pushed the checkreqs-clamp branch 2 times, most recently from 5ede9a1 to 9e78c57 Compare January 4, 2022 21:12
Crank down MAKEOPTS jobs if MAKEOPTS="-jN" is too high for the
amount of RAM available (uses amount declared as needed
in the ebuild). Typically should be ~2GB per job.

Bug: https://bugs.gentoo.org/570534
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2022-01-04 21:41 UTC
Newest commit scanned: 9f7d385
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/8c16377b20/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. no signoff One or more commits do not indicate GCO sign-off.
Projects
None yet
6 participants