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

Set up CI #134

Merged
merged 23 commits into from
Feb 3, 2024
Merged

Set up CI #134

merged 23 commits into from
Feb 3, 2024

Conversation

rffontenelle
Copy link
Contributor

Here's an work-in-progress CI setup for testing buzztrax. There is a lot of space for improvements, like the fact that it is currently failing.

It is triggered when pushing to master branch, opening a pull request to master, and started manually in Actions page.

I based this workflow file in .travis.yml and went adding dependencies the were needed to pass configure step.

Closes #126

@rffontenelle
Copy link
Contributor Author

@ensonic CI runs in my fork because I added the CI file to master branch and then went testing in another branch. How about having a initial workflow file so the pull requests trigger a CI workflow run?

For instance, a .github/workflows/build.yml containing

name: Build

on:
  pull_request:
    branches: 'master'

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
    - run: echo 'pass'

would be enough to trigger my tests.

@ensonic
Copy link
Member

ensonic commented Jan 31, 2024

@ensonic CI runs in my fork because I added the CI file to master branch and then went testing in another branch. How about having a initial workflow file so the pull requests trigger a CI workflow run?

For instance, a .github/workflows/build.yml containing

name: Build

on:
  pull_request:
    branches: 'master'

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
    - run: echo 'pass'

would be enough to trigger my tests.

Good idea. Added, you need to rebase the branch though.

@rffontenelle
Copy link
Contributor Author

@ensonic Easy fix for a merge conflict. Now that I rebased (and force-pushed due to the first commit), can you please approve workflow to be run?

@ensonic
Copy link
Member

ensonic commented Jan 31, 2024

It run. I'll check the build step that failed, probably because I locally don't build with:

	Documentation (API)             : no
	Documentation (Man)             : no
	Bindings Metadata (GIR)         : no

@rffontenelle
Copy link
Contributor Author

rffontenelle commented Jan 31, 2024

That's one thing that got me wondering: why doc/help build failed if documentation was 'no' after configure.

While this might be a good question, we probably want to enable them all, right?

@rffontenelle
Copy link
Contributor Author

btw, The PR has permissions for maintainers to include commit. Feel free to add some commits.

@rffontenelle
Copy link
Contributor Author

yay, enabling docs made the build go further:

src/lib/core/machine.c: In function ‘bt_machine_clone’:
src/lib/core/machine.c:1489:10: error: ‘machine’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1489 |   return machine;
      |          ^~~~~~~
cc1: all warnings being treated as errors
make: *** [Makefile:6443: src/lib/core/libbuzztrax_core_la-machine.lo] Error 1

@rffontenelle
Copy link
Contributor Author

Disabling --enable-debug, the build went further, but then it failed in the same C/version.entities . See the workflow run.

Making all in docs/help/bt-edit
make[2]: *** No rule to make target 'C/version.entities', needed by 'all'.  Stop.
make[2]: Entering directory '/home/runner/work/buzztrax/buzztrax/docs/help/bt-edit'
make[2]: Leaving directory '/home/runner/work/buzztrax/buzztrax/docs/help/bt-edit'
make[1]: *** [Makefile:7778: all-recursive] Error 1
make[1]: Leaving directory '/home/runner/work/buzztrax/buzztrax'
make: *** [Makefile:3262: all] Error 2

@ensonic
Copy link
Member

ensonic commented Feb 1, 2024

I've fixed the warning (that was a more recent change, so it's great if we have a ci to catch in in the PR review).
I'll now see what's causing the make error. I suspect that some dependency is wrong and depending to the parallelism applied it works or it doesn't.

@ensonic
Copy link
Member

ensonic commented Feb 1, 2024

Disabling --enable-debug, the build went further, but then it failed in the same C/version.entities . See the workflow run.

Making all in docs/help/bt-edit
make[2]: *** No rule to make target 'C/version.entities', needed by 'all'.  Stop.
make[2]: Entering directory '/home/runner/work/buzztrax/buzztrax/docs/help/bt-edit'
make[2]: Leaving directory '/home/runner/work/buzztrax/buzztrax/docs/help/bt-edit'
make[1]: *** [Makefile:7778: all-recursive] Error 1
make[1]: Leaving directory '/home/runner/work/buzztrax/buzztrax'
make: *** [Makefile:3262: all] Error 2

@dlbeswick you left a note in https://github.com/Buzztrax/buzztrax/blob/master/docs/help/bt-edit/Makefile.am#L45 on the move, but it seems to also have weird side effects.

@ensonic
Copy link
Member

ensonic commented Feb 1, 2024

I'll give up for today. Might try to revert e97b1c0 but then the out-of-srcdir build might be broken again.

@dlbeswick
Copy link
Contributor

dlbeswick commented Feb 1, 2024 via email

@dlbeswick
Copy link
Contributor

I think this was my fault. Please give this diff a go, it looks to work with both in and out of tree builds.

diff --git a/docs/help/bt-edit/Makefile.am b/docs/help/bt-edit/Makefile.am
index 7a1e4b47..6502af84 100644
--- a/docs/help/bt-edit/Makefile.am
+++ b/docs/help/bt-edit/Makefile.am
@@ -14,6 +14,13 @@ HELP_LINGUAS =
 
 DOC_MODULE = legal
 
+# Yelp help file generator expects a file "version.entities" in this directory (via HELP_MEDIA.)
+# The file usually lives in the docs folder, so a target exists to copy it here.
+# If the file doesn't exists here, then Yelp can fail with "no rule to make target 'C/version.entities".
+# So, on that target, which is generated by Yelp via HELP_FILES/HELP_MEDIA, add a dependency on the
+# target that triggers the copy of the file.
+C/version.entities: version.entities
+
 version.entities: $(abs_top_builddir)/docs/version.entities
 	@cp $< $@
 

@dlbeswick
Copy link
Contributor

Here's my configure command that I'm getting successful builds with, by the way:

./configure --enable-debug --prefix /home/david/opt/buzztrax 'CFLAGS=-Wno-error -Werror=incompatible-pointer-types -Wno-error=deprecated-declarations -Wno-error=restrict -DSTOP_PLAYBACK_FOR_UPDATES -Werror=implicit-function-declaration -Werror=format'

@rffontenelle
Copy link
Contributor Author

Thanks, it indeed fixed the issue. Now, the build went further and failed in make check. "yay, new bug!" :)

According to this workflow run, make check fails with:

FAIL: /home/runner/work/buzztrax/buzztrax/.libs/bt_core
...
FAIL: /home/runner/work/buzztrax/buzztrax/.libs/bt_gst

@rffontenelle
Copy link
Contributor Author

Now, the CI-related question is: what flags do we want enabled? Simply enable-debug seems to me as failing too early because of warnings, hence hiding actual errors.

We could e.g. add more than one build statement, to build more relaxed flags (to see if it pass) and then debug enabled.

Anyway, there is room to improve in this CI.

Also, should we consider (in another PR, maybe) a workflow for pulling translations from translationproject.org ? It could run scheduled and, in the end, create a pull request to be manually merged when wanted. Other workflow could be to manually (workflow_dispatch or on a new git tag) push new version and strings to translationproject.org. Let me know what you think

@rffontenelle
Copy link
Contributor Author

Artifacts are now being stored after rerunning tests from make check 🚀

@ensonic
Copy link
Member

ensonic commented Feb 3, 2024

I've cherry-picked the doc build fix to master and rebased. I think we can merge this. I'll spend some time in the next days to see if I can fix the tests that are broken at HEAD (or temporarily deactivate them).
@rffontenelle, any preference on squash the commits vs. keeping all individual ones?

@rffontenelle
Copy link
Contributor Author

Not squash might make the git history a noisy, but I'll leave it to you to decide.

@ensonic ensonic merged commit 92d32c2 into Buzztrax:master Feb 3, 2024
1 check failed
@ensonic
Copy link
Member

ensonic commented Feb 3, 2024

Thanks a lot!

@rffontenelle rffontenelle deleted the set-up-ci branch February 3, 2024 14:05
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.

migrate ci-setup to github-actions
3 participants