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

fix bug by building the minimal version of libhermit #160

Merged
merged 10 commits into from Jan 24, 2021

Conversation

stlankes
Copy link
Contributor

This PR should solve issue #157 In addition, I add a small test to build the minimal configuration.

@stlankes stlankes requested a review from jschwe January 20, 2021 09:27
@stlankes
Copy link
Contributor Author

@jschwe It seems to work...

@jschwe
Copy link
Contributor

jschwe commented Jan 21, 2021

@stlankes Have you tried building and running an application?
I get a lot of GP Exceptions when trying to run hello-world with no-default-features and this PR, but maybe I'm doing something wrong.
Steps:

  1. Checkout master of rusty-hermit
  2. Checkout this PR into libhermit-rs
  3. go back to rusty-hermit directory
  4. cargo build --manifest-path examples/hello_world/Cargo.toml --target x86_64-unknown-hermit --no-default-features
  5. cd loader && make && cd ..
  6. qemu-system-x86_64 -display none -smp 1 -m 64M -serial stdio -kernel loader/target/x86_64-unknown-hermit-loader/debug/rusty-loader -initrd target/x86_64-unknown-hermit/debug/pi_sequential -cpu qemu64,apic,fsgsbase,rdtscp,xsave,fxsr

@stlankes
Copy link
Contributor Author

It works only on uhyve, QEMU depends on PCI.... That is the reason, why I don't test it within GitHub Actions. In the future, I want to support of Qemu's Micro VM (https://github.com/bonzini/qemu/blob/master/docs/microvm.rst). Currently, it doesn't run on it. I have to check, why it isn't working on this setup.

@jschwe
Copy link
Contributor

jschwe commented Jan 21, 2021

Ah okay, that explains it.
I suggest to build the minimal kernel with --target x86_64-unknown-hermit-kernel and remove the std from -Z build-std=<>.
In practice, it's not really relevant of course, since we aren't using the binary, but I think it's better to stay consistent regarding the different parameters when building the kernel vs an application.
Other than that the PR looks good.

@stlankes
Copy link
Contributor Author

You are right, I fixed it

@@ -88,6 +88,9 @@ jobs:
echo "C:\Program Files\qemu" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
echo "C:\Program Files\nasm" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
if: ${{ matrix.os == 'windows-latest' }}
- name: Building minimal kernel
run:
cargo build -Z build-std=std,core,alloc,panic_abort --target x86_64-unknown-hermit --manifest-path=Cargo.toml --no-default-features
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 you need to specify the crate you want to build with --no-default-features, e.g. by adding -p rusty_demo.

@stlankes
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Jan 21, 2021
160: fix bug by building the minimal version of libhermit r=stlankes a=stlankes

This PR should solve issue #157 In addition, I add a small test to build the minimal configuration.

Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Co-authored-by: Stefan Lankes <stlankes@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 21, 2021

Build failed:

@stlankes
Copy link
Contributor Author

bors retry

bors bot added a commit that referenced this pull request Jan 21, 2021
160: fix bug by building the minimal version of libhermit r=stlankes a=stlankes

This PR should solve issue #157 In addition, I add a small test to build the minimal configuration.

Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Co-authored-by: Stefan Lankes <stlankes@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 21, 2021

Build failed:

@jschwe
Copy link
Contributor

jschwe commented Jan 21, 2021

It appears that both the integration tests and the multithreaded uhyve test is failing. The multithreaded uhyve tests crashes, but still seems to exit with 0, which causes the test to appear as passed, even though it should have failed. I'll open an issue over at uhyve to track this (Edit: We already have issue 7 tracking this)
The integration tests seem to be running fine, but uhyve (or possibly also libhermit) doesn't exit after they are finished.

I also noticed, that the way I'm dumping the stdout in the integration tests does not really result in very readable output, so I'll open a PR to improve that in the next days.

The current version of integration tests crahs sometime.
This is failure in uhyve and temporary disable these tests.
@stlankes
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Jan 22, 2021
160: fix bug by building the minimal version of libhermit r=stlankes a=stlankes

This PR should solve issue #157 In addition, I add a small test to build the minimal configuration.

Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Co-authored-by: Stefan Lankes <stlankes@users.noreply.github.com>
stlankes added a commit to stlankes/uhyve that referenced this pull request Jan 22, 2021
A valid exit code means that a thread calls the system call `exit`.
In this case, we have to terminate all threads like the behaviour of
`exit`. This current version is the reason that PR
`hermit-os/kernel#160` doesn't pass all tests.
@bors
Copy link
Contributor

bors bot commented Jan 22, 2021

Build failed:

bors bot added a commit to hermit-os/uhyve that referenced this pull request Jan 23, 2021
68: revise exit handling r=stlankes a=stlankes

A valid exit code means that a thread calls the system call `exit`. In this case, we have to terminate all threads like the behaviour of `exit`. This current version is the reason that PR `hermit-os/kernel#160` doesn't pass all tests.

Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
stlankes added a commit to stlankes/uhyve that referenced this pull request Jan 23, 2021
A valid exit code means that a thread calls the system call `exit`.
In this case, we have to terminate all threads like the behaviour of
`exit`. This current version is the reason that PR
`hermit-os/kernel#160` doesn't pass all tests.
bors bot added a commit to hermit-os/uhyve that referenced this pull request Jan 23, 2021
68: revise exit handling r=stlankes a=stlankes

A valid exit code means that a thread calls the system call `exit`. In this case, we have to terminate all threads like the behaviour of `exit`. This current version is the reason that PR `hermit-os/kernel#160` doesn't pass all tests.

Co-authored-by: Jens Breitbart <jbreitbart@gmail.com>
Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>
@stlankes
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Jan 24, 2021
160: fix bug by building the minimal version of libhermit r=stlankes a=stlankes

This PR should solve issue #157 In addition, I add a small test to build the minimal configuration.

Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Co-authored-by: Stefan Lankes <stlankes@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Jan 24, 2021

Build failed:

@stlankes
Copy link
Contributor Author

bors r+

@bors bors bot merged commit 0c2e966 into hermit-os:master Jan 24, 2021
simonschoening pushed a commit to simonschoening/libhermit-rs that referenced this pull request Aug 26, 2021
160: fix bug by building the minimal version of libhermit r=stlankes a=stlankes

This PR should solve issue hermit-os#157 In addition, I add a small test to build the minimal configuration.

Co-authored-by: Stefan Lankes <slankes@eonerc.rwth-aachen.de>
Co-authored-by: Stefan Lankes <stlankes@users.noreply.github.com>
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.

None yet

2 participants