Skip to content

ci: Use nproc over MAKEJOBS in 01_base_install #28504

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 20, 2023
Merged

ci: Use nproc over MAKEJOBS in 01_base_install #28504

merged 1 commit into from
Sep 20, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 19, 2023

Currently $MAKEJOBS is the default value in 01_base_install.sh when building the container image.

This problem can't be fixed (see below), so just use nproc for now.

Other solutions would be bad:

  • Passing in the MAKEJOBS as a dockerfile env would create a new image if the number of tasks are changed, seems verbose and confusing.
  • Leaving master as-is would leave CPUs unused if there are more than 4.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 19, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28482 (ci: use LLVM/Clang 17 in tidy job by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Sep 19, 2023
@DrahtBot DrahtBot mentioned this pull request Sep 19, 2023
Merged
@fanquake
Copy link
Member

Concept ACK

@theuni
Copy link
Member

theuni commented Sep 19, 2023

I assume I'm missing some context here, but ninja is parallel using cpu count by default. Do we really need to specify here -jFoo here? Or are we overriding to make it more parallel?

@maflcko
Copy link
Member Author

maflcko commented Sep 19, 2023

Yeah, I kept it to be consistent with make below, but happy to drop it.

fanquake
fanquake approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK - I think this is fine as-is, it's an improvement over the current state fa3b5e5

@fanquake fanquake merged commit 99ce836 into bitcoin:master Sep 20, 2023
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 25, 2023
@fanquake
Merge bitcoin#28504: ci: Use nproc over MAKEJOBS in 01_base_install
244021e
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 26, 2023
@fanquake
Merge bitcoin#28504: ci: Use nproc over MAKEJOBS in 01_base_install
fa58c72
@maflcko maflcko deleted the 2309-ci-nproc- branch September 29, 2023 14:26
@maflcko maflcko mentioned this pull request Sep 20, 2024
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maflcko @DrahtBot @fanquake @theuni