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

Support --jobserver-auth=fifo:PATH #49

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Feb 25, 2023

GNU make 4.4, released in October 20221. The jobserver defaults
to use named pipes (via mkfifo(3)) on supported platforms by
introducing a new IPC style --jobserver-auth=fifo:PATH, which PATH
is the path of fifo2.

This commit makes sure that the new style --jobserver-auth=fifo:PATH
can be forwarded to inherited processes correctly.

The support of creating a new client with named pipe will come as a
follow-up pull request.

Footnotes

  1. https://lists.gnu.org/archive/html/info-gnu/2022-10/msg00008.html

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

@kaspar030
Copy link

Thanks for working on this! I'm looking forward to the follow-up PR, any chance to (draft) PR that already?

@weihanglo
Copy link
Member Author

weihanglo commented Feb 27, 2023

Haven't yet prepared the PR. However, making fn new() defaults to fifo is not a solution in the near feature, as build script in Cargo (which is a big dependant of jobserver) could still have a chance to invoke old make. Haven't thought about how to deal with breaking changes though.

I'd like to hear what you expect :)

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks! Would it be possible to add some tests for this on CI?

src/unix.rs Outdated Show resolved Hide resolved
src/unix.rs Outdated Show resolved Hide resolved
GNU `make` 4.4, released in October 2022[^1]. The jobserver defaults
to use named pipes (via `mkfifo(3)`) on supported platforms by
introducing a new IPC style `--jobserver-auth=fifo:PATH`, which `PATH`
is the path of fifo[^2].

This commit makes sure that the new style `--jobserver-auth=fifo:PATH`
can be forwarded to inherited processes correctly.

The support of creating a new client with named pipe will come as a
follow-up pull request.

[^1]: https://lists.gnu.org/archive/html/info-gnu/2022-10/msg00008.html
[^2]: https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html
@weihanglo
Copy link
Member Author

Hi Alex. I amend the first commit to include your suggestions. Also add a new commit to test GNU Make 4.4 in CI, it compiles make from source as a temporary workaround.

@alexcrichton alexcrichton merged commit 47b5fba into rust-lang:main Feb 27, 2023
@weihanglo weihanglo deleted the jobserver-style-fifo branch February 27, 2023 23:26
@weihanglo
Copy link
Member Author

Thanks for the quick review, @alexcrichton! Sorry for bombarding your inbox again. If you've got time, it would be helpful if we have a release for this update. No hurry and take your time :)

@alexcrichton
Copy link
Member

Sure thing, done! If y'all would like I'm happy to transfer ownership of this repo as well, but it's quite low traffic so I also don't mind continuing to shepherd it.

weihanglo added a commit to weihanglo/cargo that referenced this pull request Feb 28, 2023
format!("{},{}", self.read.as_raw_fd(), self.write.as_raw_fd())
match self {
Client::Pipe { read, write } => format!("{},{}", read.as_raw_fd(), write.as_raw_fd()),
Client::Fifo { path, .. } => format!("fifo:{}", path.to_str().unwrap()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a bad idea to use the new fifo: syntax by default since this is a new syntax and not every program supports this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. That's why we only support this syntax when inheriting from the environment, and it's really in a fifo style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. That's why we only support this syntax when inheriting from the environment, and it's really in a fifo style.

Yeah this makes sense, though IMO it would be better to keep using the old syntax.

I do appreciate this new syntax since it means we no longer have to register a callback to Command::pre_exec, which blocks the use of vfork.

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.

4 participants