Skip to content

Set up CI/CD for this project, cross-compiling and publishing DOS executable using GitHub Actions Workflow#6

Open
volkertb wants to merge 15 commits intocrazii:masterfrom
volkertb:set-up-cicd
Open

Set up CI/CD for this project, cross-compiling and publishing DOS executable using GitHub Actions Workflow#6
volkertb wants to merge 15 commits intocrazii:masterfrom
volkertb:set-up-cicd

Conversation

@volkertb
Copy link

Closes #5

@volkertb
Copy link
Author

volkertb commented Dec 28, 2023

@crazii While this PR is not merged yet, you can see the results of the pipelines in my fork: https://github.com/volkertb/USBDDOS/actions

A new pipeline (Actions run) should be triggered whenever new changes are pushed to this PR.

I also fixed some issues in the Makefile and in some #include directives in the sources that DJGPP as apparently tripping over.

Your most recent commits introduced new build failures with DJGPP, since I had this working locally until I updated my branch with your most recent commits. When this PR is merged, breakage like this will be noticed quicker, since you (understandably) don't check each commit with all supported compilers while you work on the project.

Right now it's failing in DJGPP with this error, both in my local development environment and in GitHub Actions:

 ../USBDDOS/HCD/uhci.c: In function 'UHCI_StartHC':
 ../USBDDOS/HCD/uhci.c:845:14: error: unused variable 'status' [-Werror=unused-variable]
   845 |     uint16_t status = inpw((uint16_t)(pHCI->dwBaseAddress + USBSTS));
       |              ^~~~~~
 cc1: all warnings being treated as errors
 make: *** [Makefile:52: output/uhci.o] Error 1

See the pipeline log at https://github.com/volkertb/USBDDOS/actions/runs/7350805100/job/20013107796

It's a weird error though, because that variable is in fact being referenced in the following two lines:

    _LOG("UHCI start HC status: %x\n", status);
    assert(!(status & USBHCHALTED));

I did noticed the _LOG statement being greyed out as an Empty statement in my IDE. And if I'm not mistaken, assert statements are not always actually included in compilation when debugging isn't enabled. Correct? Maybe that results in neither of those statements qualifying as actually "using" that variable status.

@volkertb volkertb mentioned this pull request Dec 28, 2023
@crazii
Copy link
Owner

crazii commented Dec 28, 2023

The errors are fixed.

@volkertb
Copy link
Author

Thanks. I just rebased this branch on your recent upstream commits, and indeed, compilation is successful again. 👍

The build job passes, but the publish job is failing now, but that's due to a bug in my work, not in your source code.

I'm confident I can fix that without too much effort.

As for also setting up CI/CD for Open Watcom compilation: I'd still like to do that, but I'll open a separate PR for that, once this one is merged.

@volkertb
Copy link
Author

volkertb commented Dec 28, 2023

Pipeline is now working, both the building and the publishing. See the following:

We might want to replace the TODO in the release notes with something else. @crazii feel free to suggest something to put there. Thanks! 🙂

…rkflow, replace TODO placeholder in release notes template with brief explanation about difference between the two artifacts
…ince the compilation is passing locally with `act`, but failing in actual GitHub Actions for some reason.
…n separate steps, to ease build failure troubleshooting
… that it's only failing in GitHub Actions when building with `make DEBUG=1` (which passes locally with `act`, yet fails in actual GitHub Actions for some reason)
…o verify that it's only failing in GitHub Actions when building with `make DEBUG=1` (which passes locally with `act`, yet fails in actual GitHub Actions for some reason)"

This reverts commit c4a6558.
@volkertb
Copy link
Author

The debug-enabled variant is failing with a compilation error in GitHub Actions, even though it builds fine locally.

See this post for more details about that.

@volkertb
Copy link
Author

volkertb commented Jan 7, 2024

The relocation truncated to fit error while building the debug-enabled variant might be due to the GitHub build runners having not enough RAM available for the build. Perhaps I can work around that by enabling a swap file during the build or something.

@volkertb
Copy link
Author

volkertb commented Jan 7, 2024

Then again, the standard GitHub-hosted runners apparently have 7GB of RAM, and I successfully ran the GitHub Action with act on a Linux PC with 8GB of RAM. So that makes it less likely that this was the cause. Unless that one extra GB of RAM made just the difference between success and failure when building USBDDOS with debug enabled.

@crazii
Copy link
Owner

crazii commented Jan 8, 2024

The relocation truncated to fit error while building the debug-enabled variant might be due to the GitHub build runners having not enough RAM available for the build. Perhaps I can work around that by enabling a swap file during the build or something.

Glad to hear that. Congrats!

@crazii
Copy link
Owner

crazii commented Jan 8, 2024

Sorry I just pushed my changes, I should've merge it first. the unstable connection to github makes my anxious, editing readme.md online or pushing codes. damn.

@volkertb
Copy link
Author

volkertb commented Jan 8, 2024

No, that's okay. The problem with the debug builds failing with that relocation truncated to fit error still hasn't been resolved. There are two options at this point:

  • Merge it with the debug builds disabled in GitHub Actions for now (so the debug builds aren't built automatically on each commit or merge to master/main, only the "release" (non-debug) build. And then try to troubleshoot the failing debug builds and open a separate PR to re-enable it later, once fixed.
  • Spend some more time trying to get the debug builds to succeed in this PR, and wait with merging until then.

What do you think?

(By the way, don't worry about any changes you merge or commit in the meantime. It's trivial to rebase PRs, especially since the PR changes different files than the stuff you're merging or committing in the meantime.)

@crazii
Copy link
Owner

crazii commented Jan 8, 2024

Cool, thanks. I will also add git tags string into the make file.

@crazii
Copy link
Owner

crazii commented Jan 9, 2024

Now the git commit hash is added into build string, I'm not using tags I think the commit hash is good enough.
I also added it for Open watcom, but it just need a shell wrapper, if you also setup for open watcom, please run the ./owcmake.sh directly without invoking wmake. The shell script will pass git commit hash to makefile.

@crazii
Copy link
Owner

crazii commented Feb 13, 2024

Pipeline is now working, both the building and the publishing. See the following:

We might want to replace the TODO in the release notes with something else. @crazii feel free to suggest something to put there. Thanks! 🙂

Todo is a development notes that is not for release purpose.
It's a list that we needed not forget 'to do it in the future'.

@crazii
Copy link
Owner

crazii commented Feb 13, 2024

No, that's okay. The problem with the debug builds failing with that relocation truncated to fit error still hasn't been resolved. There are two options at this point:

  • Merge it with the debug builds disabled in GitHub Actions for now (so the debug builds aren't built automatically on each commit or merge to master/main, only the "release" (non-debug) build. And then try to troubleshoot the failing debug builds and open a separate PR to re-enable it later, once fixed.
  • Spend some more time trying to get the debug builds to succeed in this PR, and wait with merging until then.

What do you think?

(By the way, don't worry about any changes you merge or commit in the meantime. It's trivial to rebase PRs, especially since the PR changes different files than the stuff you're merging or committing in the meantime.)

Either way will be ok, the error is related to the building environment but we don't know what triggers it. We can remove all packages to get a clean env to see if it still happens. If it still happens then we have to leave it for now.

@crazii
Copy link
Owner

crazii commented Feb 13, 2024

And sorry that when I was in the middle of coding, I become ignoring because I was focusing on the coding problem (it uses too many of my brain cells. :)

So forgive me if I didn't reply you in time. I didn't mean it.

@volkertb
Copy link
Author

Hi again!

No need to apologize, since I've been off the radar myself, these last two weeks. (A little bicycle accident, still typing with one hand 😅)

I'll need to find some time to process your feedback and dust off this PR. Thanks for understanding. 🙂

@crazii
Copy link
Owner

crazii commented Feb 27, 2024

OK, cool.
Get well soon!

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.

Github Actions

2 participants