provide thread name to OS for Solarish systems #62309
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
ping from triage @jlevon can you fix the license/copyright stuff? thanks |
Dylan-DPC-zz
added
S-waiting-on-author
|
@Dylan-DPC I already answered there, that there is nothing to fix. The original commenter seems to have confused copyright with licensing. I marked the conversation as Resolved, perhaps that what you were waiting for? |
|
We prefer to keep the tree without Copyright markers for anyone/any organization; I am not a lawyer so I can't personally say whether the copyright marker you've left is separate from licensing or such (I definitely see your point, though!). Is it possible for you to remove the "Copyright" header on that file? |
|
Hi Mark, is that policy written up somewhere? As I already mentioned, COPYRIGHT explicitly mentions files may contain Copyright markers, and I couldn't find any other mention. Is there some specific issue with the above? Joyent corporate policy requires a copyright notice to present (see https://github.com/joyent/rfd/blob/master/rfd/0164/README.md for details). This is presumably for legal reasons way beyond my ken as just another engineer. |
|
@Mark-Simulacrum does @jlevon's response clear things up? Would love to see this merged as setting thread names on illumos is currently a no-op, and having them would help with debugging. |
|
Ah, missed that previous comment. I'm not sure if the policy is specifically recorded anywhere, and suspect that it probably isn't. I'm going to somewhat blindly r? @alexcrichton who I believe might have better answers for what we can/can't do here, or at least know who to ask. |
|
Thanks for the cc here. I won't really pretend I know how best to handle this myself, but @jlevon there's probably two ways to go about this. One is as been mentioned it'll be easiest if you're ok removing the comment and we can r+/merge. That follows the pattern of all the other PRs we receive, so it's easy to put in the "yep this is fine" bucket. The other strategy is that if you'd prefer this is left in we'll probably need to double check with someone (probably lawyers of some form). The "most accessible" ones we know of are from Mozilla, and they often take months to get back to us on issues like this. From my perspective it'd be great if we could make this as standard as other PRs are, but if you'd like I can try to set things in motion to get a consulation on the legal side of things to see what the implications of a comment are like this for us and how we'd handle it. This isn't a situation we've encountered in rust-lang/rust before (contributors asking to place license headers in files they modify) AFAIK and may take some time to navigate. In any case I leave it up to you to let me know how you'd like to proceed! |
|
Thanks for the reply Alex. FWIW it's a copyright notice not a license header - very different things. Of course I can understand you might want to check with someone knowledgeable on the legal side, so please do so. This is our corporate policy as I mentioned, as an individual contributor I have no control over this. However, I will also re-raise this issue internally and report back. |
|
Ah sorry yeah my legalese is very imprecise and flat out wrong most of the time. The main point I wanted to convey is that this sort of comment in a PR to rust-lang/rust is something that's nonstandard for us and we do not have a process in place for handling it. It'd be easiest if this could be removed but if it can't it'll likely just block this from landing for a significant amount of time, and I'd hate to prevent improvements from happening sooner! |
|
Yep, understood, thanks. |
|
ping from triage @jlevon, any updates on this? |
|
Yes, good news, we have permission to drop the Copyright attribution lines for Rust repositories. |
Mark-Simulacrum
added
S-waiting-on-review
| #[cfg(target_os = "solaris")] | ||
| pub fn set_name(name: &CStr) { | ||
| weak! { | ||
| fn pthread_setname_np( |
There was a problem hiding this comment.
We could use oneline:
fn pthread_setname_np(libc::pthread_t, *const libc::c_char) -> libc::c_int
|
📌 Commit 6be2d9a has been approved by |
bors
added
S-waiting-on-bors
provide thread name to OS for Solarish systems Fixes #62302 Passes a Linux bootstrap build. python x.py test src/tools/tidy happy. I tested this with a small test binary that spawns a few threads, and verified that: - on an illumos system lacking the libc function, the binary runs but no OS-level thread names are set - on an illumos system with the feature, the binary runs, and the thread names are visible and correct under tools like MDB, pstack, core dump, etc.
|
☀️ Test successful - checks-azure |
Fixes #62302
Passes a Linux bootstrap build. python x.py test src/tools/tidy happy.
I tested this with a small test binary that spawns a few threads, and verified
that:
thread names are set
visible and correct under tools like MDB, pstack, core dump, etc.