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

ecm boot test #77

Merged
merged 13 commits into from Apr 2, 2023
Merged

ecm boot test #77

merged 13 commits into from Apr 2, 2023

Conversation

ecm-pushbx
Copy link
Contributor

A very simple boot test, no drivers, no XMS HMA or UMA, no shell. The test script is based on parts from ldosboot/test/test.sh and ldebug/source/makqimg.sh

This PR is to kick off the first CI run and see if anything is missing. As @andrewbird suggested I hope that the PR's CI will pick up the changes I made to the CI itself.

@ecm-pushbx
Copy link
Contributor Author

Partial success: qemu and mtools appear to be installed as per my changes to the .github/ file.

@ecm-pushbx
Copy link
Contributor Author

Not sure why it doesn't find ./test.sh, on our system it works fine. @andrewbird any ideas? https://github.com/FDOS/kernel/runs/6546040210?check_suite_focus=true lists:

./ci_test.sh: 22: ./test.sh: not found
GCC boot test failed
Error: Process completed with exit code 2.

This is the first time it actually lists an error message at all, don't know either why it didn't do that before.

@andrewbird
Copy link
Contributor

Does it have bash?

@ecm-pushbx
Copy link
Contributor Author

Does it have bash?

Worth an attempt, thanks.

@ecm-pushbx
Copy link
Contributor Author

Still fails the same way after adding a bash package name to the .github

@ecm-pushbx
Copy link
Contributor Author

Latest attempt shows we have /bin/bash where I expect it:

-rwxr-xr-x 1 runner docker 3307 May 22 20:39 test.sh
-rwxr-xr-x 1 runner docker 3307 May 22 20:39 test.sh
-rwxr-xr-x 1 root root 1183448 Apr 18 09:14 /bin/bash
./ci_test.sh: 23: ./test.sh: not found
GCC boot test failed
Error: Process completed with exit code 2.

@ecm-pushbx
Copy link
Contributor Author

Checking whether I need bash for running ci_test.sh itself maybe.

@ecm-pushbx
Copy link
Contributor Author

That leaves us with a more useful error message:

./ci_test.sh: ./test.sh: /bin/bash^M: bad interpreter: No such file or directory
GCC boot test failed
Error: Process completed with exit code 2.

@andrewbird
Copy link
Contributor

Does the script have Unix line endings?

@ecm-pushbx
Copy link
Contributor Author

Does the script have Unix line endings?

It does on my system. However, git internally seems to store it with CR LF and I don't know how to override this. (Cloning the git repo locally also leads to the error.)

@andrewbird
Copy link
Contributor

Add it to .gitattributes

@ecm-pushbx
Copy link
Contributor Author

Add it to .gitattributes

I was wondering why autocrlf and safecrlf in the ~/.gitconfig seemed not to have any effect!

@ecm-pushbx
Copy link
Contributor Author

Thanks, that did it! The test appears to work now. I will clean up the commits in this PR some day, @PerditionC I recommend not merging it before then.

@PerditionC
Copy link
Contributor

Ok, thanks for working on it

@ecm-pushbx
Copy link
Contributor Author

Moved the historical attempt to https://github.com/ecm-pushbx/fdkernel/commits/ecm-boot-test-old

I'm not sure about using -e test for the git clean commands, I believe we could drop it and let git delete all untracked files. Thoughts?

With that question answered I think the initial simple test is ready to be merged.

@ecm-pushbx
Copy link
Contributor Author

Ping, is this still wanted? Any additional wishes?

@PerditionC
Copy link
Contributor

Yes

@andrewbird
Copy link
Contributor

@ecm-pushbx it's been a while since you proposed this and the Github Actions logs have expired. Could you trigger a rerun (it might be a good idea to rebase and force push the branch which will accomplish this) please?

@ecm-pushbx
Copy link
Contributor Author

@ecm-pushbx it's been a while since you proposed this and the Github Actions logs have expired. Could you trigger a rerun (it might be a good idea to rebase and force push the branch which will accomplish this) please?

Done.

@ecm-pushbx
Copy link
Contributor Author

Hmm, I do not see the CI is running my tests?

@ecm-pushbx
Copy link
Contributor Author

Hmm, I do not see the CI is running my tests?

It did actually, just had to find how to view the log.

@ecm-pushbx
Copy link
Contributor Author

I don't have the time right now but one of the commits seems to have disappeared. I will announce if/when I fixed it.

@andrewbird
Copy link
Contributor

Disappeared during rebase? I'm no expert but I seem to remember being able to find lost commits with git reflog show, then you can git cherry-pick them back.

@ecm-pushbx
Copy link
Contributor Author

Disappeared during rebase?

Yes.

I'm no expert but I seem to remember being able to find lost commits with git reflog show, then you can git cherry-pick them back.

I seem to have fixed it by doing this:

git clone https://github.com/FDOS/kernel citest3
cd citest3
git checkout 8e6fc98a94cd0fd
git pull ../citest2.bak/
git rebase -i origin
git branch ecm-boot-test
git push https://ecm-pushbx@github.com/ecm-pushbx/fdkernel.git +ecm-boot-test

8e6fc98 was the original commit on which I developed the PR's commits. citest2.bak was a backup I created using git clone before attempting the rebase the first time today.

@ecm-pushbx
Copy link
Contributor Author

Now the CI results seem to be missing. Or do I just not know where to look?

@ecm-pushbx
Copy link
Contributor Author

Not pretty but the empty commit made it run CI as intended. Do you want me to remove that one later?

@andrewbird
Copy link
Contributor

You can rerun your own jobs in https://github.com/ecm-pushbx/fdkernel/actions, but the proposed merge commit job can only be rerun from the webui by FDOS owner. However, you can brute-force trigger it yourself by closing and then reopening the PR, but don't push to that PR whilst it's in the closed state or the reopen button will be greyed out.

@andrewbird
Copy link
Contributor

Does it still run okay, it's been a while?

@andrewbird
Copy link
Contributor

Does it still run okay, it's been a while?

What I'm trying to say is perhaps you could rebase off current HEAD, force push and see the test run again? As it is the Github Actions log for this PR has expired and nobody can see what it does any more.

@ecm-pushbx
Copy link
Contributor Author

ecm-pushbx commented Mar 25, 2023

Yes, I'll get around to this soon. I also want to update the additional sources I included from my repos.

@andrewbird
Copy link
Contributor

Cool, I just didn't want you to think my comment was being negative towards your patch.

@ecm-pushbx
Copy link
Contributor Author

Okay, I rebased the branch using the cleaner approach in this answer and the tests both ran fine, both on our server and in github's CI. I will update from my repos now.

@andrewbird
Copy link
Contributor

Looks good!

@ecm-pushbx
Copy link
Contributor Author

I updated lmacros, ldosboot, and bootimg, and tweaked the test script to use _BOOTPATCHFILE now. This allows arbitrary file system parameters to be used (just the type of FAT12, FAT16, or FAT32 needs to be selected correctly).

@ecm-pushbx
Copy link
Contributor Author

I'll wait for a few days, if no one has any more comments then I'll see about merging this.

@ecm-pushbx ecm-pushbx merged commit 4973031 into FDOS:master Apr 2, 2023
1 check passed
@ecm-pushbx
Copy link
Contributor Author

Merged now. I used github's rebase and merge button but it seems to have inserted a Commit and CommitDate field to each commit, containing my github handle and today's datetime. I'll look into pushing to the main branch manually in the future.

@andrewbird
Copy link
Contributor

I like just like to see plain merge commits myself, or it's difficult to see here what has been merged or not and from where.

@ecm-pushbx
Copy link
Contributor Author

Does that leave the individual commits alone ie not add the CommitDate/Commit fields to the individual commits?

@andrewbird
Copy link
Contributor

Looking at this one here dosemu2/dosemu2@c5d95fd the merge itself has the current date/time, but if you follow the parent link back into the branch they have an earlier date and author. Is that what you are looking for?

@ecm-pushbx
Copy link
Contributor Author

Yes, I think that's it. Thanks!

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

3 participants