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

Adding support for QNX OS #6421

Merged
merged 4 commits into from
May 14, 2024
Merged

Conversation

SebastianSchildt
Copy link
Contributor

This enables tokio to be used on QNX.
It also depends on

tokio-rs/mio#1766

@AkhilTThomas I added you to the fork, so you can write there as well

The changes are rather trivial, the main thing is a i32/u32 type mismatch that is handled

Not so sure what is expected/can done in terms of testing, as I assume there are no QNX machines in CI? What are the expectations here?

Once this is in acceptable form, would we need to backport it on one of the many version branches?
As just tokio users we would appreciate some form of QNX support being available out of the box on released tokio versions, (even if would be hidden behind a "here be dragons, we have no idea what this does" kind-of flag )

Comment on lines 675 to 677
#[cfg(target_os = "nto")]
self.std.uid(id as i32);
#[cfg(not(target_os = "nto"))]
self.std.uid(id);
Copy link
Contributor

@Darksonn Darksonn Mar 22, 2024

Choose a reason for hiding this comment

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

Suggested change
#[cfg(target_os = "nto")]
self.std.uid(id as i32);
#[cfg(not(target_os = "nto"))]
self.std.uid(id);
#[cfg(target_os = "nto")]
let id = id as i32;
self.std.uid(id);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in @AkhilTThomas commit 29cfcf8

Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix it everywhere.

@Darksonn Darksonn added the A-tokio Area: The main tokio crate label Mar 22, 2024
@Darksonn
Copy link
Contributor

Thanks for the PR. On the assumption that the mio PR is merged, then I am happy to add it here as well.

Once this is in acceptable form, would we need to backport it on one of the many version branches?

No, it will just go in with the next minor release and be available starting from Tokio v1.37 (or later depending on timing). Unlike mio, we don't have multiple supported major versions.

Not so sure what is expected/can done in terms of testing, as I assume there are no QNX machines in CI? What are the expectations here?

Adding support in CI is preferred but not required. Often, cargo check is possible even if there are no QNX machines in CI if we use cross-compilation.

(even if would be hidden behind a "here be dragons, we have no idea what this does" kind-of flag )

Tokio has a list of platforms with guaranteed support and a bunch of other platforms we support on a best-effort basis. This will not be something we guarantee ongoing support for, but I do not mind adding it.

Copy link

@gh-tr gh-tr left a comment

Choose a reason for hiding this comment

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

Curious about the bsd versus netbsd path.

@@ -161,7 +161,7 @@ pub(crate) mod impl_netbsd {
}
}

#[cfg(any(target_os = "dragonfly", target_os = "freebsd"))]
#[cfg(any(target_os = "dragonfly", target_os = "freebsd", target_os = "nto"))]
Copy link

Choose a reason for hiding this comment

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

Looking at my diff for tokio 1.18.2, I had enabled the impl_netbsd path, not the impl_bsd path. I presumably did this as it includes the PID whereas getpeereid() does not. Any argument why the getpeereid() path should be used over this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what is the correct answer for this target.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, using the impl_netbsd is better. I will update the PR

@SebastianSchildt
Copy link
Contributor Author

Running test suite would need nix-rust/nix#2055 not a blocker now for "best effort" QNX support, but preferably should be solved

@SteveLauC
Copy link
Contributor

Hi, Nix maintainer here, that Nix PR wasn't merged because we think it might not be worth considering how small the user group is, but if tokio wants this target support and it is blocked by that Nix PR, then I am willing to merge it.

@Darksonn
Copy link
Contributor

That's needed to compile the test-suite, but would it actually enable us to run the tests? Can we run this OS in CI?

@SebastianSchildt
Copy link
Contributor Author

Hi @Darksonn : Yes, it would enable to compile, run tests. Which is good. Running all this in CI seems more complicated, see also my comment on mio tokio-rs/mio#1766 (comment)

Again, as QNX users we appreciate everything that moves QNX support a bit closer to be on-par with other Tier 1 targets, but there is still some way to go. How far individual projects are willing to go to support this -for now- niche target is of course up to the communities. First step would be having some "not guaranteed to work/best effort code", that gives some amount of out-of -the-box functionality. Obviously, for all users it would be good to be able to run the test suite at least locally, especially since CI testing might still be a slightly harder nut to crack.

@Darksonn
Copy link
Contributor

This has been merged for mio. Any updates on the PR for Tokio?

@AkhilTThomas
Copy link
Contributor

This has been merged for mio. Any updates on the PR for Tokio?

Hi @Darksonn We are waiting for the merge of the nix fix and one on the signal-hook-registry as well.

@Darksonn
Copy link
Contributor

Is there a link to the signal-hook-registry change?

@AkhilTThomas
Copy link
Contributor

Is there a link to the signal-hook-registry change?

Unfortunately not yet, its stuck with an internal clearance 😢. But its a one liner with a suppression for SA_RESTART being unavailable for nto target.

@SebastianSchildt
Copy link
Contributor Author

SebastianSchildt commented Apr 12, 2024

Is there a link to the signal-hook-registry change?

Unfortunately not yet, its stuck with an internal clearance 😢.

That it was, it isn't anymore vorner/signal-hook#158

@Darksonn Darksonn mentioned this pull request May 3, 2024
@Darksonn
Copy link
Contributor

Darksonn commented May 3, 2024

Any updates here? I'm okay with merging it before it goes into other dependencies, since the change is so small. (But I do have a pending change request.)

@SebastianSchildt
Copy link
Contributor Author

Hi, it is not forgotten. May take a few days before sitting at a suitable computer. Just to be sure, the change you are referring is the shorter/ more "elegant" way of doing the id as i32;, right?

@Darksonn
Copy link
Contributor

Darksonn commented May 5, 2024

Okay, that's fine.

And yes, that's what I meant.

@Darksonn
Copy link
Contributor

You probably need to merge in master for CI to pass.

@github-actions github-actions bot added R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR R-loom-sync Run loom sync tests on this PR R-loom-time-driver Run loom time driver tests on this PR labels May 11, 2024
@Darksonn
Copy link
Contributor

It looks like your branch is messed up.

@github-actions github-actions bot removed R-loom-multi-thread-alt Run loom multi-thread alt tests on this PR R-loom-sync Run loom sync tests on this PR R-loom-time-driver Run loom time driver tests on this PR R-loom-current-thread Run loom current-thread tests on this PR R-loom-multi-thread Run loom multi-thread tests on this PR labels May 12, 2024
@AkhilTThomas
Copy link
Contributor

It looks like your branch is messed up.

My bad. Fixed it

tokio/src/process/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Alice Ryhl <aliceryhl@google.com>
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just one question about the integer cast.

Comment on lines 672 to 677
#[cfg(unix)]
#[cfg_attr(docsrs, doc(cfg(unix)))]
pub fn uid(&mut self, id: u32) -> &mut Command {
#[cfg(target_os = "nto")]
let id = id as i32;
self.std.uid(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if uid is given an id that becomes negative when cast to i32?

Copy link
Contributor

Choose a reason for hiding this comment

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

I set it to i32 as the uid args are typedef to INT_32 in the core files. Digging a bit deeper into the manual i see that QNX recommends that negative values are not used!

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thank you.

@Darksonn Darksonn merged commit 227979f into tokio-rs:master May 14, 2024
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants