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 HermitCore support even if it doesn't have a UNIX interface #1292

Merged
merged 9 commits into from
Mar 3, 2019

Conversation

stlankes
Copy link
Contributor

@stlankes stlankes commented Mar 2, 2019

Currently, we redefine the interface between the HermitCore kernel (https://hermitcore.org) and Rust’s standard library. In the future, it will not depend on a POSIX-compatible C library. Consequently, we add the support of HermitCore if the "unix" environment isn’t set. It will be great to integrate this patch because it would simplify our development. The classical interface is still supported and part of the subdirectory "unix".

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (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.

Copy link
Contributor

@gnzlbg gnzlbg 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 for the PR fellow Aachener!

I think you should add two hermit targets (x86_64 and aarch64) to the ci/build.sh file in this PR. You can use xargo to cross-compile libc to hermit by providing two json target specification files.

That's far from what's required to be sure that this change is correct, but it is the minimum required to prevent other people, including yourself, from ensuring that libc "at least compiles" for these targets.

If you need help with adding these build tests to CI, i'm on Discord and Zulip.

src/lib.rs Outdated Show resolved Hide resolved
src/hermit/mod.rs Show resolved Hide resolved
src/hermit/mod.rs Show resolved Hide resolved
@stlankes
Copy link
Contributor Author

stlankes commented Mar 2, 2019

You are right. I oversaw the redefinition and the typo.

Until now, HermitCore war missing in ci/build.sh. Do you have an tutorial, how I could add HermitCore? I know xargo and use it for our project.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 2, 2019

There is no tutorial, but that you are already using xargo is great. I assume that you pass xargo the target specification files (e.g. x86_64-...-hermit.json and aarch64-...-hermit.json), is that correct?

If so, it should be enough to add the targets here: https://github.com/rust-lang/libc/blob/master/ci/build.sh#L169

And to copy the json files in the libc/ci directory.

You probably want to try that locally. For that the best you can do is put them as the first two targets of that environment variable, and comment this other loop (https://github.com/rust-lang/libc/blob/master/ci/build.sh#L163) so that only the "no core" targets get tested.

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

So this LGTM. We'll see what CI says but the targets have a spec in https://github.com/rust-lang/rust/tree/master/src/librustc_target/spec so they should "just work".

Please feel free to ping me when CI is green, or if any of the build jobs stalls and needs to be restarted :)

@@ -19,7 +19,9 @@
pub type c_long = i64;
pub type c_ulong = u64;

#[allow(unused)]
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is weird, the only way in which these could be unused is if they are not exported by the library API, right?

src/unix/hermit/mod.rs Outdated Show resolved Hide resolved
@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 3, 2019

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 3, 2019

📌 Commit 14353e8 has been approved by gnzlbg

@bors
Copy link
Contributor

bors commented Mar 3, 2019

⌛ Testing commit 14353e8 with merge 8d94e00...

bors added a commit that referenced this pull request Mar 3, 2019
add HermitCore support even if it doesn't have a UNIX interface

Currently, we redefine the interface between the HermitCore kernel (https://hermitcore.org) and Rust’s standard library. In the future, it will not depend on a POSIX-compatible C library. Consequently, we add the support of HermitCore if the "unix" environment isn’t set. It will be great to integrate this patch because it would simplify our development. The classical interface is still supported and part of the subdirectory "unix".
@bors
Copy link
Contributor

bors commented Mar 3, 2019

☀️ Test successful - checks-cirrus, checks-travis, status-appveyor
Approved by: gnzlbg
Pushing 8d94e00 to master...

@bors bors merged commit 14353e8 into rust-lang:master Mar 3, 2019
@stlankes stlankes mentioned this pull request Mar 5, 2019
@mkroening mkroening mentioned this pull request Sep 11, 2023
bors added a commit that referenced this pull request Sep 21, 2023
Hermit updates

This PR does three things related to the Hermit target:

1.  Close #3318.

    Non-unix Hermit has been supported since #1292 and unix Hermit does not exist anymore.

2.  Update Hermit docs.

    [HermitCore/RustyHermit has been renamed to just Hermit.](https://rust-osdev.com/this-month/2023-08/#hermit-os-kernel)
    I took the chance to simplify the Hermit module's docs.

3.  Add the `riscv64gc-unknown-hermit` target to docs and CI (rust-lang/rust#114004).
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