Skip to content

Conversation

@flejz
Copy link
Contributor

@flejz flejz commented Oct 6, 2025

Checklist

Not all of these might apply to your change but the more you are able to check
the easier it will be to get your contribution merged.

  • CI passes
  • Description of proposed change
  • Documentation (README, docs/, man pages) is updated

Purpose:
Compile for windows so there is official support.
Release tested and working for targets:

GOOS=windows
GOARCH=amd64
GOARCH=arm64 

https://github.com/flejz/checkmake/actions/runs/18276474308/job/52029646621

package cloud build is failing but it is out of the scope of this PR

@flejz flejz marked this pull request as ready for review October 6, 2025 09:46
Copy link
Collaborator

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @flejz !

In general, it sounds good to add support for building on Windows.

Since I don't have a Windows machine to build on, it would be nice if you could add a ci test to build and test on Windows (To verify your changes and protect from regressions). Currently, the CI only tests on Linux.

Additionally, you seem to have done a 0.2.3release in your own fork and added a release notes file to this PR. I would request you to hold off such change until we do an official release here. And please remove the release file from this PR and remove mention of release from the commit message.

I'll need to review the changes to packages.yml thoroughly and will get back.

@flejz
Copy link
Contributor Author

flejz commented Oct 7, 2025

The "release" I made was only really to test the pipeline. I will remove it from my fork given the intention of keeping it in its official repo.

Will do the changes requested asap.

@flejz
Copy link
Contributor Author

flejz commented Oct 11, 2025

@obnoxxx now app is building and testing also o Windows with minimal changes in the CI pipeline.

https://github.com/flejz/checkmake/actions/runs/18430886793/job/52518037763

Waiting feedback.

Copy link
Collaborator

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

Thank you for the update @flejz !

This looks a lot better now.

But apart from the request for the Makefile change inline below I have a few general requests:

  • please use git rebase origin/main instead of git merge.
  • would you please sign off your commits by adding a Signed-off-by line (E. g. using git commit -s?
  • Please squash your two commits so that there is no separate commit for the removal of the release notes added in the first commit.

Copy link
Collaborator

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

one more request.

@flejz
Copy link
Contributor Author

flejz commented Oct 13, 2025

Will apply the changes asap then let you know.

flejz added 2 commits October 13, 2025 11:49
Signed-off-by: flejz <flejz@protonmail.com>
Signed-off-by: flejz <flejz@protonmail.com>
Copy link
Collaborator

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

Thank you for the update, @flejz !

This looks good now! 🎉
Thank you for your contribution!

@obnoxxx obnoxxx merged commit ba0a410 into checkmake:main Oct 13, 2025
3 checks passed
@obnoxxx obnoxxx mentioned this pull request Oct 14, 2025
@flejz
Copy link
Contributor Author

flejz commented Oct 21, 2025

@obnoxxx when can we expect a new release that includes all latest changes + official Windows support?

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.

2 participants