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

Add QNX/nto support #325

Merged
merged 2 commits into from
Dec 24, 2022
Merged

Conversation

flba-eb
Copy link
Contributor

@flba-eb flba-eb commented Nov 14, 2022

This patch adds support to the QNX/nto operating system. This target is supported by Rust: rust-lang/rust#102701.

cc @gh-tr

@flba-eb flba-eb closed this Nov 14, 2022
@flba-eb flba-eb reopened this Nov 14, 2022
@newpavlov
Copy link
Member

Is it possible to add a build job to our CI for this target?

Copy link
Member

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

You can add this target to our CI by adding x86_64-pc-nto-qnx7.1.0 after this line.

src/lib.rs Outdated Show resolved Hide resolved
src/use_file.rs Show resolved Hide resolved
src/util_libc.rs Show resolved Hide resolved
@josephlr josephlr added the waiting on author Changes requested or more information is needed label Nov 15, 2022
@flba-eb
Copy link
Contributor Author

flba-eb commented Nov 15, 2022

Is it possible to add a build job to our CI for this target?

I would love to have QNX/nto builds in CI, but I think it is not feasible right now because a license key is required. Also, the license seems not to work well in containers but works best with real hardware. I will forward this question to check if there is a possibility.

@josephlr
Copy link
Member

Is it possible to add a build job to our CI for this target?

I would love to have QNX/nto builds in CI, but I think it is not feasible right now because a license key is required. Also, the license seems not to work well in containers but works best with real hardware. I will forward this question to check if there is a possibility.

That makes sense, we don't need to run the code, The line I suggested adding would only build the code, not execute it. Obviously actually running on qnx is better, but just building in our CI ensures that we don't inadvertently break things.

@flba-eb
Copy link
Contributor Author

flba-eb commented Nov 15, 2022

Is it possible to add a build job to our CI for this target?

I would love to have QNX/nto builds in CI, but I think it is not feasible right now because a license key is required. Also, the license seems not to work well in containers but works best with real hardware. I will forward this question to check if there is a possibility.

That makes sense, we don't need to run the code, The line I suggested adding would only build the code, not execute it. Obviously actually running on qnx is better, but just building in our CI ensures that we don't inadvertently break things.

Not using the official QNX/nto toolchain would be tricky, because a special linker is used: https://github.com/flba-eb/rust-lang-fork/blob/master/compiler/rustc_target/src/spec/nto_qnx_base.rs#L12
Also, we would need to link against a libc.so which exposes the symbol libc::__get_errno_ptr.

I suggest to go without the tests.

@josephlr
Copy link
Member

josephlr commented Nov 15, 2022

Not using the official QNX/nto toolchain would be tricky, because a special linker is used:

I realize that we cannot link without access to an OS specific toolchain. This is not the only target where we have this issue. That is why the line I suggested adding just compiles the code, without linking or running it.

@newpavlov
Copy link
Member

Compilation includes linking, no? Maybe we can use cargo check for the most basic testing?

@flba-eb
Copy link
Contributor Author

flba-eb commented Nov 16, 2022

I tested a bit with it but removed the test code again. We don't have (upstreamed) libc support yet (work in progress), which is why even cargo check does not work yet.

@josephlr
Copy link
Member

I tested a bit with it but removed the test code again. We don't have (upstreamed) libc support yet (work in progress), which is why even cargo check does not work yet.

That's good to know, and one of the reasons we like to build code for new targets. cargo build and cargo check will only compile the rust code, it shouldn't need to link anything.

@newpavlov would you be okay accepting code that depends on not-yet-upstreamed libc support? Or should we just wait for the upstream libc changes before merging this?

@newpavlov
Copy link
Member

@josephlr
I think it's better to wait. If someone needs the support right away, then they simply can temporarily patch getrandom dependency in addition to libc.

@josephlr
Copy link
Member

@josephlr I think it's better to wait. If someone needs the support right away, then they simply can temporarily patch getrandom dependency in addition to libc.

Sounds reasonable.

@flba-eb feel free to ping this issue once the libc support for QNX/nto gets merged. Then I'd be happy to merge this.

@josephlr josephlr added waiting on upstream Waiting for upstream changes in an external project and removed waiting on author Changes requested or more information is needed labels Nov 17, 2022
@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 21, 2022

@josephlr Ping! QNX support has been added to libc: rust-lang/libc#3038

@josephlr
Copy link
Member

@josephlr Ping! QNX support has been added to libc: rust-lang/libc#3038

Awesome!! Can you bump the libc version the Cargo.toml file to include the release that has that PR, and add aarch64-unknown-nto-qnx710 to tests.yml here

@flba-eb
Copy link
Contributor Author

flba-eb commented Dec 22, 2022

@josephlr , done, please check. Check runs locally.

@josephlr josephlr removed the waiting on upstream Waiting for upstream changes in an external project label Dec 24, 2022
@josephlr
Copy link
Member

The QNX tests are passing so I'm going to merge this. I'll fix the other tests in a followup PR.

@josephlr josephlr merged commit beb65e2 into rust-random:master Dec 24, 2022
@flba-eb flba-eb deleted the add_qnx_nto_support branch December 24, 2022 08:46
@newpavlov newpavlov mentioned this pull request Apr 2, 2023
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.

3 participants