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.

Sign up for GitHub

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

Jump to bottom

Base0x10 patch 1 #389

Merged
merged 7 commits into from
May 13, 2020
Merged

Base0x10 patch 1 #389

merged 7 commits into from
May 13, 2020

Conversation

base0x10
Copy link

@base0x10 base0x10 commented Sep 19, 2019 •
edited
Loading

Summary of this Pull Request (PR)

This PR only updates some outdated documentation, and
fixes the vagrantscript

Intent for your PR

Choose one (Mandatory):

  • This PR is for a code-review and is intended to get feedback, but not to be pulled yet.
  • [ X] This PR is mature, and ready to be integrated into the repo.

Reviewers (Mandatory):

(Specify @<github.com username(s)> of the reviewers. Ex: @user1, @user2)
@gparmer
@Others

Code Quality

There is no new code in this PR, only updates to docs and vagrantfile options

As part of this pull request, I've considered the following:

Style:

  • Comments adhere to the Style Guide (SG)
  • Spacing adhere's to the SG
  • Naming adhere's to the SG
  • All other aspects of the SG are adhered to, or exceptions are justified in this pull request
  • I have run the auto formatter on my code before submitting this PR (see doc/auto_formatter.md for instructions)

Code Craftsmanship:

  • [ X] I've made an attempt to remove all redundant code
  • [ ]X I've considered ways in which my changes might impact existing code, and cleaned it up
  • [ X] I've formatted the code in an effort to make it easier to read (proper error handling, function use, etc...)
  • [ X] I've commented appropriately where code is tricky
  • [ X] I agree that there is no "throw-away" code, and that code in this PR is of high quality

Testing

I've tested the code using the following test programs (provide list here):

Tested that changes to vagrantfile work on a new install. Will require rebuilding
a vm to pass ioapic

  • micro_booter
  • unit_pingpong
  • unit_schedtests
  • ...(add others here)

@base0x10
Update README.md with missing deps
517fe2c 
G++ is required and doesn't exist by default on all installations
@base0x10
Update Vagrantfile to pass correct flags to vmm
e998170 
VirtualBox requires ioapic to be enabled for the vcpus to have effect.  
without it, the number of CPUs is always 1.
@base0x10
Add missing deps to vm provision script
acb0e49 
g++ is required and does not come installed
Rust toolchain is installed, set to nighly, and deps are installed
@base0x10
Update documentation on running tests/microbooter
3cc6506 
Looks like microboot was removed.  Update documentation to not ref it, and instead point to kernel_test.sh
@base0x10 base0x10 requested a review from gparmer September 19, 2019 18:35
@gparmer
Copy link
Collaborator

gparmer commented Mar 8, 2020

I dropped the ball on this, and you've since moved to docker. I presume that closing this is the correct course. I can reopen, if you want to work from here with the docker stuff.

@gparmer gparmer closed this Mar 8, 2020
@Others
Copy link
Contributor

Others commented Mar 8, 2020

@gparmer would it be possible to reopen and merge this? I use vagrant and have to manually install g++ every time. Also it’s good to document the changed “default” runscript

@gparmer gparmer reopened this Mar 8, 2020
@gparmer
Copy link
Collaborator

gparmer commented Mar 8, 2020

@Others Absolutely. I fixed the remaining merge conflict. Can you attest to this being good to go? I don't have the setup to test this. If you're good with it, it can be merged.

@gparmer
Copy link
Collaborator

gparmer commented Apr 10, 2020

@Others @base0x10 Ping. Love to merge, and don't have the setup to test that it works. Mind attesting it works?

base0x10 and others added 2 commits April 15, 2020 13:09
@base0x10
Swith vagrant provision script from super user to user
ff6f834 
By default vagrant provision scripts run as super user.  Rust
build system does not need priv to be installed.  If it is
installed as super user, it is installed in /root/.cargo and
is inaccessable without sudo.

Other commands within provision script were already using sudo
@base0x10
Merge pull request #400 from base0x10/base0x10-patch-1
ccccc7c 
Swith vagrant provision script from super user to user
@base0x10
Copy link
Author

@gparmer Tested and works. kernel_tests.sh and unit_schedtests.sh both run successfully without reporting any issues. I didn't compare performance against baremetal or other virtualization platforms. Issue described below isn't blocking and doesn't seem to have to do with vagrant, but rather with rust nightly. I don't think it should block merging this PR, but up to you.

@Others there is currently an issue with rust related to this issue. Does not prevent building or running the system without rust. I have attached the output: issue.txt

@gparmer
Copy link
Collaborator

gparmer commented May 13, 2020

I swear I pulled this a week ago? I need to go back through the records ;-(

@gparmer gparmer merged commit 28d0e42 into ppos May 13, 2020
@gparmer
Copy link
Collaborator

gparmer commented May 13, 2020

Thanks for this update!!!

@Others
Copy link
Contributor

Others commented May 13, 2020

@base0x10 regarding the rust issue, did you use nightly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gparmer gparmer Awaiting requested review from gparmer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@base0x10 @gparmer @Others