Base0x10 patch 1 - Githubissues

gwsystems / composite

A component-based OS

composite.seas.gwu.edu

185 stars 70 forks source link

Base0x10 patch 1 #389

Closed base0x10 closed 4 years ago

base0x10 commented 5 years ago

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:

[ ] 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)

gparmer commented 4 years ago

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.

Others commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

base0x10 commented 4 years ago

@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 commented 4 years ago

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

gparmer commented 4 years ago

Thanks for this update!!!

Others commented 4 years ago

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