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

fixes for building on windows/msys2 #1408

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

giuspen
Copy link

@giuspen giuspen commented Feb 2, 2025

  • version locally the script geany-plugins-release.py so far hosted in the wiki
  • applied fix for building without signing (missing key/certificate in the filesystem)
  • applied fix to geanygendoc/docs/Makefile.am
  • applied fix to lsp/src/lsp-server.c
  • applied fix for languages binaries not being generated
  • changed version num to 2.1

@b4n
Copy link
Member

b4n commented Feb 2, 2025

Could you please split each item to its own commit? We prefer having smaller more specific commits leading to the final stage. Of course each commit should be complete in regard of what it is meant to achieve, but each of these should try and be small. It also gives you the opportunity to actually explain why you changed something, both to help reviewers and future understanding of why a change has been made.

geanygendoc/docs/Makefile.am Outdated Show resolved Hide resolved
po/LINGUAS Outdated Show resolved Hide resolved
@@ -45,6 +45,7 @@
# include "spawn/lspunixoutputstream.h"
#endif

#include <unistd.h>
Copy link
Member

Choose a reason for hiding this comment

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

Is this change on purpose?

If so, @techee should add it upstream as well.

Copy link
Member

Choose a reason for hiding this comment

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

No problem to add but is it actually needed @giuspen ? The plugin seemed to build without it on my machine at least.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @techee have you tried to build in msys2 or you are referring to cross compiling? I couldn't on windows/mays2 without this patch but I can revert and reapply when I build locally anyway, not a big deal

Copy link
Member

Choose a reason for hiding this comment

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

I tried it but it's possible it was before making some changes in lsp-server.c that require this header. I was just wondering if you ran into any problems without it.

No problem to add this include and make the corresponding change in the lsp repo.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, it's most probably because of the getpid() call, whose declaration indeed is in unistd.h.

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.

4 participants