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

CMake build system + msys2 windows binaries CI (fixes #11, fixes #148) #177

Closed
wants to merge 11 commits into from

Conversation

hartwork
Copy link

@hartwork hartwork commented Jan 31, 2022

Fixes #11
Fixes #148

@hartwork hartwork force-pushed the cmake-linux-msys2 branch 2 times, most recently from fe847b0 to b95bdfc Compare February 13, 2022 01:58
@hartwork hartwork changed the title [W.I.P.] CMake build system + msys2 windows binaries CI [W.I.P.] CMake build system + msys2 windows binaries CI (fixes #148) Feb 13, 2022
@hartwork hartwork changed the title [W.I.P.] CMake build system + msys2 windows binaries CI (fixes #148) [W.I.P.] CMake build system + msys2 windows binaries CI (fixes #11, fixes #148) Feb 13, 2022
@hartwork hartwork force-pushed the cmake-linux-msys2 branch 2 times, most recently from 2906671 to 08c801b Compare February 22, 2022 18:18
@hartwork hartwork changed the title [W.I.P.] CMake build system + msys2 windows binaries CI (fixes #11, fixes #148) CMake build system + msys2 windows binaries CI (fixes #11, fixes #148) Feb 22, 2022
@hartwork
Copy link
Author

@gkdr, I made this^^ ready for review now.

The pre-merge GitHub Action logs can be viewed at https://github.com/hartwork/lurch/actions .

Moving CodeCov integration to GitHub Actions will need a new secret on the repository. We can do that migration before or after merge of this pull request.

PS: AppVeyor CI seems to also need disabling on their website, deleting the file alone doesn't make it stop.

@hartwork hartwork force-pushed the cmake-linux-msys2 branch 3 times, most recently from 1fc7b2a to 3383abd Compare March 1, 2022 01:50
Copy link
Owner

@gkdr gkdr left a comment

Choose a reason for hiding this comment

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

it looks like the windows result is not loadable yet.
report from a friend using windows, for both versions, all the zip contents unpacked into the plugins dir:

(21:41:43) plugins: probing C:\Users\USER\AppData\Roaming\.purple\plugins\lurch.dll
(21:41:43) plugins: C:\Users\USER\AppData\Roaming\.purple\plugins\lurch.dll is not loadable: `C:\Users\USER\AppData\Roaming\.purple\plugins\lurch.dll': The specified module could not be found.

.github/workflows/windows.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@hartwork hartwork force-pushed the cmake-linux-msys2 branch 3 times, most recently from 0e87f83 to 4d1c180 Compare March 3, 2022 21:25
@hartwork
Copy link
Author

hartwork commented Mar 3, 2022

it looks like the windows result is not loadable yet. report from a friend using windows, for both versions, all the zip contents unpacked into the plugins dir:

(21:41:43) plugins: probing C:\Users\USER\AppData\Roaming\.purple\plugins\lurch.dll
(21:41:43) plugins: C:\Users\USER\AppData\Roaming\.purple\plugins\lurch.dll is not loadable: `C:\Users\USER\AppData\Roaming\.purple\plugins\lurch.dll': The specified module could not be found.

@gkdr I notice now that for Windows test test/test_lurch_loadable.c passes in (SHARED=OFF) but fails in (SHARED=ON), with the same error. We ignore tests failures on Windows and that's why I didn't notice earlier. It could be that we need to copy the DLL closure into the build folder before running tests, but then that would not explain the problem on your friend's machine, provided they copied all DLLs in there. I'm also wondering if maybe the jabber plugin needs to be in the same folder or so.

@hartwork hartwork force-pushed the cmake-linux-msys2 branch from 4d1c180 to dea5c85 Compare March 4, 2022 03:29
@hartwork
Copy link
Author

hartwork commented Mar 4, 2022

It could be that we need to copy the DLL closure into the build folder before running tests

@gkdr it turned out that doing that^^ fixed the issue in the CI. It also implies that your friend did not have all needed files at the right places. My guess is still the jabber plugin DLL.

@hartwork hartwork force-pushed the cmake-linux-msys2 branch from dea5c85 to 76ef2ce Compare March 4, 2022 03:37
@gkdr
Copy link
Owner

gkdr commented Mar 4, 2022

is it maybe related to setting the rpath like here?

lurch/Makefile

Line 90 in 71c806b

LDFLAGS += -ldl -lm $(PKGCFG_L) $(LJABBER) -Wl,-rpath,$(PURPLE_PLUGIN_DIR)

@hartwork
Copy link
Author

hartwork commented Mar 4, 2022

is it maybe related to setting the rpath like here?

lurch/Makefile

Line 90 in 71c806b

LDFLAGS += -ldl -lm $(PKGCFG_L) $(LJABBER) -Wl,-rpath,$(PURPLE_PLUGIN_DIR)

@gkdr excellent point, let me look into that more.

@hartwork hartwork force-pushed the cmake-linux-msys2 branch from 76ef2ce to d784899 Compare March 6, 2022 04:00
@hartwork
Copy link
Author

hartwork commented Mar 6, 2022

is it maybe related to setting the rpath like here?

lurch/Makefile

Line 90 in 71c806b

LDFLAGS += -ldl -lm $(PKGCFG_L) $(LJABBER) -Wl,-rpath,$(PURPLE_PLUGIN_DIR)

@gkdr excellent point, let me look into that more.

@gkdr thanks for bringing RPATH to my attention, this is important and was not working as needed. What I found out is that:

  • RPATH is a topic on Linux but not Windows.
  • By default, CMake builts with an explicit RPATH but the resets it in the binaries at install time.
  • As a result, current make install would install a lurch.so that is not loadable by Pidgin while our own make install-home is loadable by Pidgin just fine (because it still has the RPATH set (or not yet reset)).
  • If we make CMake use the installation-time RPATH during build, the test suite will fail.
  • If we make CMake NOT use the installation-time RPATH during build, then make-install home will install binaries with the wrong rpath.

Because of all that, what I have done now is this:

  • Make make install-home install files libaxc.* and libomemo.* (when using a bundled copy in shared library mode) as well
  • Split off target lurch_home just for make install-home with the right RPATH set at build time.

Does that approach work for you?

@EionRobb
Copy link

@gkdr you keep running away from #pidgin on librachat before you see my replies :)

It looks like the lurch dll's are trying to load libxml2-2.dll weirdly, trying to load DllMain() as a funtion, rather than linking to the functions in libxml:
image

I'm using https://github.com/lucasg/Dependencies and https://www.dependencywalker.com to scan for dependencies

Compare to other dlls that get loaded which look like
image

Also the plugin is trying to load in libsqlite3-0.dll, but Pidgin ships with sqlite3.dll, so might be superflous?

@gkdr
Copy link
Owner

gkdr commented Apr 13, 2022

thanks a lot, @EionRobb! that does indeed look a bit weird and is a very good starting point :D

@EionRobb
Copy link

Also, (and its probably unrelated) just running something else through GDB and saw:

(21:56:04) plugins: probing C:\Program Files (x86)\Pidgin\plugins\lurch_semi_shared.dll
Dwarf Error: wrong version in compilation unit header (is 5, should be 2, 3, or 4) [in module C:\Program Files (x86)\Pidgin\plugins\lurch_semi_shared.dll]
Dwarf Error: wrong version in compilation unit header (is 5, should be 2, 3, or 4) [in module C:\Program Files (x86)\Pidgin\libsqlite3-0.dll]
(21:56:05) plugins: C:\Program Files (x86)\Pidgin\plugins\lurch_semi_shared.dll is not loadable: `C:\Program Files (x86)\Pidgin\plugins\lurch_semi_shared.dll': The specified procedure could not be found.
Dwarf Error: wrong version in compilation unit header (is 5, should be 2, 3, or 4) [in module C:\Program Files (x86)\Pidgin\plugins\lurch_semi_shared.dll]
Dwarf Error: wrong version in compilation unit header (is 5, should be 2, 3, or 4) [in module C:\Program Files (x86)\Pidgin\libsqlite3-0.dll]

@xvitaly
Copy link

xvitaly commented Feb 7, 2023

@gkdr Can you merge this PR?

CMakeLists.txt Outdated Show resolved Hide resolved
@Neustradamus
Copy link

@gkdr, @xvitaly: Have you looked the last changes from @hartwork?

@Neustradamus
Copy link

@gkdr: Any news on this old important PR?

@gkdr
Copy link
Owner

gkdr commented Apr 27, 2024

Hi @Neustradamus! At this point, as far as I know, this does not work, as in the resulting plugin won't load on Windows. Neither of us has a Windows computer to test this, so I don't know how to develop it.

@hartwork
Copy link
Author

hartwork commented Apr 27, 2024

@gkdr as long as a re-run gets green CI, we could merge it and ask for pull requests from people actually using Windows. What do you think?

@EionRobb
Copy link

Hi @Neustradamus! At this point, as far as I know, this does not work, as in the resulting plugin won't load on Windows. Neither of us has a Windows computer to test this, so I don't know how to develop it.

I can take another look.

@EionRobb
Copy link

I couldn't get it to build on Windows, as there's no mingw-w64-i686-pidgin package in pacman any more; pacman gives an error "error: target not found: mingw-w64-i686-pidgin". Looks like there's only 64-bit packages now? https://packages.msys2.org/base/mingw-w64-pidgin

Tested by building on Windows with act -P windows-2019=-self-hosted --pull=false -W .github/workflows/windows.yml (using https://github.com/nektos/act for local action runners)

@hartwork
Copy link
Author

@EionRobb thanks for trying and the hint, I have adjusted for 64bit now and the CI is now back to green.

@gkdr
Copy link
Owner

gkdr commented Apr 30, 2024

@hartwork What value is added if this does not add Windows support?

@EionRobb Thank you!

@hartwork
Copy link
Author

@hartwork What value is added if this does not add Windows support?

@gkdr

  • the pull request also replaces zombie AppVeyor CI by GitHub Actions
  • the pull request also migrates to a CMake build system e.g. to
    • pave the way for compilation for Windows
    • get out of the hard-to-get-right hand-written makefile zone into a more high level language
    • add high-level switches to the build system for end users and packagers
    • add proper error reporting for missing dependencies whereas the makefile lets you run right into compile errors
  • the pull request also adds test test_lurch_loadable.c
  • building for Windows is a precondition for running on Windows, not the other way around; with no Windows support in current dev, adding a Windows build covered by CI is a big step forward for first class Windows support, even if it is not working for everyone yet in practice.

Would you like to continue with the current Makefile on dev, instead?

@Neustradamus
Copy link

@gkdr: Have you seen the latest @hartwork comment?

@gkdr
Copy link
Owner

gkdr commented Aug 31, 2024

@hartwork Thanks for the reminder. Especially the test for loadability seems nice. I tried looking at CMake for a while and to be honest I just cannot grasp it. I am hesitant to add it because currently I cannot use it. Do you have a few pointers? The CMake homepage didn't seem to help.

@hartwork
Copy link
Author

hartwork commented Sep 2, 2024

Hi @gkdr,

you and I migrated both of Lurch's dependencies axc and libomemo to CMake in 2022…

…, and at the time you considered that "a major step towards the automated windows build of lurch". My e-mails from back then indicate that it was our call on 2022-01-20 where I showed building blocks of CMake to you.

Now after two and a half years of this pull request being mostly ignored, I have no motivation left for helping you value and merge this pull request any more. It's not been fun to wait for you forever after all the effort I put into this, it's been too long, and I will not make the same mistake twice. I am now closing this pull request, it's about time. Thanks for your understanding.

Best, Sebastian

@hartwork hartwork closed this Sep 2, 2024
@gkdr
Copy link
Owner

gkdr commented Sep 2, 2024

Yes, the goal was an automated build for Windows, like this PR's title also says. We didn't quite get there, that's why it's not merged. Thanks for all your time and effort up to this point. If you leave your branch up, I might pick it up someday, if you don't mind.

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.

Switch to CMake Target for build based on shared libs
5 participants