-
Notifications
You must be signed in to change notification settings - Fork 11
os9 comand logical sector format fix #31
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
Conversation
strickyak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| -L../librbf -L../libmisc -L../libnative -L../libsys \ | ||
| -ltoolshed -lcoco -lcecb -ldecb -lrbf -lmisc -lnative -lsys -lm | ||
| -L../librbf -L../libnative -L../libmisc -L../libsys \ | ||
| -ltoolshed -lcoco -lcecb -ldecb -lrbf -lnative -lmisc -lsys -lm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
Is this necessary to fix the regression, or just better for some reason?
Did it fail to compile without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It failed on GitHub's build farm because of the misordering of these two libraries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@strickyak click the red X, you can see build log. Its line 8 that matters, as previously _native_open wasn't being used but now that it is, depends on libmisc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Craig @callsop . I used the similar tools at Google very effectively for years, but Github still feels a bit alien to me. I'm learning!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to merge this with my checked-in PR,
QSY to #34
This simplifies how the sectorsTrack0 variable is used, giving it an "unset" value of 0. It moves a test that forks the "os9 format" command into a new file unittest/os9commandtest.c Fixes the order of libraries, as needed in automated testing. This PR was originally #31 but the fundamental bug fix was made in #29 so it has been adapted.
* fix Track0 sector size in formatting Hard Drive. It should not be different from the sector size on other tracks. Also a unit test that breaks when the bug exists. FIXES #28 * Tim's refactor of track0 sectors for os9 format This simplifies how the sectorsTrack0 variable is used, giving it an "unset" value of 0. It moves a test that forks the "os9 format" command into a new file unittest/os9commandtest.c Fixes the order of libraries, as needed in automated testing. This PR was originally #31 but the fundamental bug fix was made in #29 so it has been adapted.
|
This was superseded by PR #34. |
This is an alternate way to fix issue #28.
Also I created a seperate os9 command test file.