Skip to content

ci: add Cygwin build workflow #2066

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.

Sign up for GitHub

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

Jump to bottom
Merged
jubalh merged 1 commit into from
Jan 24, 2026
Merged

ci: add Cygwin build workflow #2066

jubalh merged 1 commit into from
Jan 24, 2026

Conversation

@ritesh006
Copy link
Contributor

@ritesh006 ritesh006 commented Sep 13, 2025

  • I ran valgrind when using my new feature

How to test the functionality

  • step 1
  • step 2

@ritesh006 ritesh006 force-pushed the ci/cygwin-build branch 12 times, most recently from 7c1c710 to ae479f8 Compare September 13, 2025 09:23
@jubalh jubalh added the tests label Sep 13, 2025
@jubalh jubalh added this to the next milestone Sep 13, 2025
@jubalh
Copy link
Member

jubalh commented Sep 13, 2025

Awesome! This looks like a good start :)

You could have set this PR to draft PR and then to regular PR when you want us to review.
But it is fine as it is now. Just request a review from us once you are done and got everything to work :)

@ritesh006
Copy link
Contributor Author

ritesh006 commented Sep 13, 2025

Awesome! This looks like a good start :)

You could have set this PR to draft PR and then to regular PR when you want us to review. But it is fine as it is now. Just request a review from us once you are done and got everything to work :)

sorry I dont understand what is draft pr?

@jubalh
Copy link
Member

jubalh commented Sep 13, 2025

It's just a way to signal whether your pull request is still work in progress or ready for review by maintainers.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request

@ritesh006 ritesh006 force-pushed the ci/cygwin-build branch 3 times, most recently from c9cc0c4 to 178d3f4 Compare September 13, 2025 12:34
@ritesh006
Copy link
Contributor Author

ritesh006 commented Sep 13, 2025

cygwin-build build success @jubalh could you please review this

jubalh
jubalh requested changes Sep 15, 2025
View reviewed changes
Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you need to clean up your history.
The commit Merge branch 'master' into ci/cygwin-build is not needed and just clutters the history. The other 3 commits can be squashed into one.

@ritesh006
Copy link
Contributor Author

ritesh006 commented Sep 18, 2025

Hi @jubalh due to family loss I am in hometown as soon as I reach to my place would like to work start again. Thank You

@jubalh
Copy link
Member

jubalh commented Sep 18, 2025

My condolences. Take your time!

@ritesh006
Copy link
Contributor Author

ritesh006 commented Sep 18, 2025 •
edited
Loading

Now you need to clean up your history. The commit Merge branch 'master' into ci/cygwin-build is not needed and just clutters the history. The other 3 commits can be squashed into one.

Thanks I cleaned up the branch and squashed the changes into a single commit.

What I did: added a GitHub Actions workflow to check that Profanity builds on Cygwin (runs on windows-latest). The job installs the needed Cygwin packages and runs autoreconf -fi, ./configure, and make. I also forced the steps to run under Cygwin’s bash and handled temp-script paths and CRLF issues so the job should run more reliably on the runner.

Next steps I’m thinking of (if you’re OK with this landing):

enable make check later if tests are stable on Cygwin,

add a MSYS2/MINGW64 job for native Windows builds,

upload build logs/artifacts on failures to make debugging easier.

I renamed the workflow to Cygwin to match the other job names. Please take a look and let me know if you want any changes.

@jubalh
Copy link
Member

jubalh commented Sep 18, 2025

Sounds good!

Regarding:

enable make check later if tests are stable on Cygwin,

The tests do not succeed there currently?

@jubalh
Copy link
Member

jubalh commented Jan 7, 2026

Unfortunately now there is again a merge commit. Like already mentioned in #2066 (review) we dont like them :)
A rebase on master would have been more appropriate.
Please get rid of that second commit.

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 7, 2026

Unfortunately now there is again a merge commit. Like already mentioned in #2066 (review) we dont like them :) A rebase on master would have been more appropriate. Please get rid of that second commit.

Please check now is everything ok.

@sjaeckel
Copy link
Member

sjaeckel commented Jan 7, 2026 •
edited
Loading

You simply removed the unittests ... I'm not a big fan.

Is there no cmocka available or what prevents us from building and running them?

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 7, 2026

On Cygwin the tests failed earlier because the cmocka dependency is not available in the job.
That is why make check failed, so I removed the step for now.
We can enable tests again once cmocka works properly on Cygwin, either by adding a package or building it from source. I’m happy to look into that in a follow-up PR.

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 7, 2026

I added libcmocka-devel so the tests can be supported in the future.
I also tried enabling make check, but Cygwin fails with:

make: *** No rule to make target 'check'. Stop.

So it seems this build configuration does not expose a make check target yet.
At the moment the workflow just verifies the build, and cmocka is installed so we can enable tests later.

What would you suggest, should we keep it as a build-only job for now, or look into wiring the tests on Cygwin in a follow-up PR?

@ritesh006 ritesh006 requested review from jubalh and sjaeckel January 7, 2026 17:52
@jubalh
Copy link
Member

jubalh commented Jan 9, 2026

I'm fine with having this PR just adding building on Cygwin to CI and another PR which then enables the tests. But if you have time you can also enable them in this PR. What's more urgent though is that the CI doesn't fail :) Which currently it still does. And I saw that there are a couple of comments from @sjaeckel above. It seems some of his questions haven't been answered by you yet or am I missing something?

@sjaeckel
Copy link
Member

sjaeckel commented Jan 9, 2026

I'm fine with having this PR just adding building on Cygwin to CI and another PR which then enables the tests.

Fine by me.

What's more urgent though is that the CI doesn't fail :) Which currently it still does.

This.

And I saw that there are a couple of comments from @sjaeckel above. It seems some of his questions haven't been answered by you yet or am I missing something?

And this.

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 12, 2026 •
edited
Loading

I fixed the CI so it no longer fails by removing the make check step.
The Cygwin job now only verifies that Profanity builds, which we agreed is fine for this PR.

I also simplified the workflow and addressed the review comments:

  • removed unused / debug-only steps,

  • removed test-related comments and logic,

  • kept a single default Cygwin bash wrapper for all steps.

Test support on Cygwin can be added in a follow-up PR once a make check target is available.

Please let me know if anything else should be adjusted.

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 12, 2026

I addressed all remaining review comments:

  • simplified the workflow and removed debug-only steps,

  • removed redundant shell overrides,

  • added a short comment explaining the Windows/Cygwin wrapper and bootstrap.sh normalization,

  • removed test-related logic so CI no longer fails.

The PR now only ensures Profanity builds on Cygwin. Test support can be added in a follow-up PR.

Thanks for the review!

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 20, 2026

@jubalh could you please review this

@jubalh
Copy link
Member

jubalh commented Jan 20, 2026

I didn't forget about it. I was messaging jaeckel today asking him whether he is okay with your comments. Once he replied we can continue.
Because for example: #2066 (comment) didnt get a comment.

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 20, 2026

I didn't forget about it. I was messaging jaeckel today asking him whether he is okay with your comments. Once he replied we can continue. Because for example: #2066 (comment) didnt get a comment.

Thank You @jubalh for quick response.

jubalh
jubalh requested changes Jan 22, 2026
View reviewed changes
Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned previously: please stop adding merge commits. There is no need for this. This branch doesn't have any conflicts with master. And even if it would have you need to rebase and not merge.

Your answer to the other question satisfies me and I'll merge after this.

@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 24, 2026

As mentioned previously: please stop adding merge commits. There is no need for this. This branch doesn't have any conflicts with master. And even if it would have you need to rebase and not merge.

Your answer to the other question satisfies me and I'll merge after this.

sorry for that. I have given the reply. Please let me know the further steps

@sjaeckel
Copy link
Member

sjaeckel commented Jan 24, 2026

As mentioned previously: please stop adding merge commits.

Please let me know the further steps

Please

  1. read my last comment and do whatever you think is reasonable.
  2. rebase on top of master and force-push.

sjaeckel
sjaeckel approved these changes Jan 24, 2026
View reviewed changes
Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me. Can be merged after rebasing.

@ritesh006
ci: add Cygwin build workflow
f21ce24 
Add a GitHub Actions workflow to build with Cygwin on Windows.
Fixes issues by forcing Cygwin bash for all steps, handling temporary script paths,
stripping CRLF line endings, and enabling igncr.
@jubalh jubalh merged commit f5eccea into profanity-im:master Jan 24, 2026
6 checks passed
@jubalh
Copy link
Member

jubalh commented Jan 24, 2026

Thanks!

@ritesh006 ritesh006 deleted the ci/cygwin-build branch January 25, 2026 14:00
@ritesh006
Copy link
Contributor Author

ritesh006 commented Jan 25, 2026 •
edited
Loading

Thanks again! Let me know if there’s anything you’d suggest I work on next. @jubalh @sjaeckel

@jubalh
Copy link
Member

jubalh commented Jan 25, 2026

You can pick any task from the issue tracker. Or think about something yourself. Feel free to contact us or open an issue to discuss it first in case you want to be sure it gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sjaeckel sjaeckel sjaeckel approved these changes

@jubalh jubalh jubalh approved these changes

Assignees

@ritesh006 ritesh006

Labels

tests

Projects

None yet

Milestone

next

Development

Successfully merging this pull request may close these issues.

3 participants

@ritesh006 @jubalh @sjaeckel