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

make: pass make jobserver to cargo #19203

Merged
merged 1 commit into from
Jan 26, 2023
Merged

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Jan 26, 2023

Contribution description

For cargo to honour our make's jobserver, it's recipe line needs to be prefixed with +.
Make >= 4.4 currently also needs the --jobserver-style=pipe argument. (Our CI is using 4.3).

Testing procedure

make -Ctests/rust-hello-world clean all -j2

On master, observe as many rustc processes (e.g., in top) as there are CPUs. With this PR, there should only be two.

(should your system have make 4.4, try make --jobserver-style=pipe -Ctests/rust-hello-world clean all -j2)

Issues/PRs references

Fixes #19045.

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 26, 2023
@kaspar030 kaspar030 requested review from chrysn and maribu and removed request for chrysn January 26, 2023 19:56
@github-actions github-actions bot added the Area: build system Area: Build system label Jan 26, 2023
@riot-ci
Copy link

riot-ci commented Jan 26, 2023

Murdock results

✔️ PASSED

6fb226d make: pass make jobserver to cargo

Success Failures Total Runtime
6796 0 6796 07m:36s

Artifacts

@chrysn
Copy link
Member

chrysn commented Jan 26, 2023

While I generally subscribe to "Comments should assume the user knows the language", not only is this the first time I see + in many years of Makefile use, but I also haven't even managed to find the documentation for them (although the page on .ONESHELL (TIL) mentions them as "special prefix characters". Thus, I'd welcome a comment being added that explains what's happening here.

Given that + also makes the line exempt from make --just-print, do we need to pass some --dry-run to cargo when an actual --just-print / -n is active? Or do we just not support that mode of Make?

@chrysn
Copy link
Member

chrysn commented Jan 26, 2023

And: Can this go away again when #19045 is fixed? (If so, the comment should say so too).

@kaspar030
Copy link
Contributor Author

Thus, I'd welcome a comment being added that explains what's happening here.

I added one.

Here's an explanation of the jobserver:

https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html

And: Can this go away again when #19045 is fixed? (If so, the comment should say so too).

Nope. If the "+" is not used, make passes invalid fds, hinting the jobserver is unavailable / should not be used.

Given that + also makes the line exempt from make --just-print, do we need to pass some --dry-run to cargo when an actual --just-print / -n is active? Or do we just not support that mode of Make?

hm, make -n fails horribly for me with/without this change. I'd say we don't support that mode.
By spec, jobserver clients should look for "-n" in the makeflags and do the right thing.

Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Works as advertised. Thanks for fixing this!

@kaspar030
Copy link
Contributor Author

I added one.

btw, that makefile is IMO a nice example of why make is not the nicest language to write build systems in. It does what needs to be done, but it gets ugly...

image

@kaspar030
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Jan 26, 2023

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@bors
Copy link
Contributor

bors bot commented Jan 26, 2023

Build succeeded:

@bors bors bot merged commit 2492e21 into RIOT-OS:master Jan 26, 2023
@kaspar030 kaspar030 deleted the cargo_jobserver branch February 6, 2023 09:03
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust building ignores Makefile job control
4 participants