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

Fix make resource failure when cc parallel is enabled #985

Merged
merged 13 commits into from
Mar 4, 2024

Conversation

NobodyXu
Copy link
Collaborator

@NobodyXu NobodyXu commented Mar 2, 2024

Fixed #962

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
And `rm -r job_token`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Attempt to fix CI msrv failure

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Since we uses a function from 0.1.20

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Change it to an async function that returns `Result<JobToken, Error>`

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu marked this pull request as ready for review March 2, 2024 04:06
@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Mar 2, 2024

Verified locally by building cargo-near, that this PR indeed fixed #962

@weihanglo
Copy link
Member

Thank you for your hard work chasing this down!

Just fly-by. Could we have one or two simple paragraphs describing what was wrong and how this PR gets it correct?

@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Mar 2, 2024

Yes, so the problem is that make < 4.3 pass jobserver using annoymous pipe fd, and thus they share the file description between parent and child processes, e.g. flags O_NONBLOCK are shared among make, cargo, build.rs, etc.

When cc::Build::compile in one build.rs sets O_NONBLOCK on the pipe, all other processes, including make, will also affected by this change.

Since make < 4.3 is not able to handle jobserver pipe with O_NONBLOCK set, it will fail.

make >= 4.3 uses O_NONBLOCK flag itself and it supports the named fifo mode, which is more robust than annoymous
pipe, because they don't share the file description.

Edit:

This PR fixed this, by reverting to use crate jobserver, which does not set the O_NONBLOCK and spawns a thread to avoid blocking instead.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Collaborator Author

NobodyXu commented Mar 3, 2024

Note that jobserver currently has a problem: It doesn't clone the anonymous pipe passed via env.

So if someone calls Build::compile, creates a jobserver::Client and drop it, then calling Build::compile again will use the now closed pipe fd.

Since fd is reused, this end up creates some really strange bug (this happened in opencv before twistedfall/opencv-rust#480 , they fixed it by using jobslot, which does clone the annoymous pipe fd)

I've created rust-lang/jobserver-rs#57 but it has not been merged yet

Plus jobserver now has msrv 1.63, and cc is still on 1.56

@n-eq
Copy link

n-eq commented Mar 4, 2024

Confirmed this fixes the issue on our side, both for MacOS and iOS targets. Thanks @NobodyXu ! Looking forward to the next release

@NobodyXu NobodyXu mentioned this pull request Mar 4, 2024
@NobodyXu NobodyXu merged commit a7aa5e3 into main Mar 4, 2024
42 checks passed
@NobodyXu NobodyXu deleted the fix/job-token-make branch March 4, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make resource failure when cc parallel is enabled
4 participants