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

[CI] Run on Windows #48

Merged
merged 32 commits into from
Aug 10, 2022
Merged

[CI] Run on Windows #48

merged 32 commits into from
Aug 10, 2022

Conversation

giordano
Copy link
Member

Test in view of #43, I don't know if this will work, let's see

@giordano giordano added the technical Technical and meta issues, not related to physics but infrastructure. label Jul 25, 2022
@giordano giordano force-pushed the mg/ci-windows-macos branch 2 times, most recently from fd2c3f2 to aa6f9cb Compare July 25, 2022 11:19
@giordano
Copy link
Member Author

Some progress: compilation is successful on Windows, but tests are failing because tdms.exe can't be found.

@t-young31 t-young31 linked an issue Jul 25, 2022 that may be closed by this pull request
@giordano
Copy link
Member Author

giordano commented Jul 25, 2022

I'm completely stuck at this point and I have no clue of what to do. There are two orthogonal issues:

  1. tdms.exe is built successfully but it isn't installed. This could be solved by running cmake --install . --config ${{ matrix.build_type }}, but this would install the program in a directory (I think it was something like C:\Program Files (x86)\tdms\bin) which isn't in the PATH and so the executable can't be found anyway, which isn't much progress. I see two options:
    1. Search the executable also in a Release/ subdirectory of the current folder. I don't like this option much, but might be OK-ish (but still very meh) for the purpose of running the tests in CI
    2. Add the installation directory to the PATH. I'd say this is the "proper" solution in the Unix world, but I'm not sure people on Windows are accustomed to fiddling with these settings
  2. For the life of me I can't make the dynamic loader on Windows find the needed dynamic libraries, like fftw3.dll or libmat.dll/libmex.dll. I tried to add the relevant directories to PATH (the Windows equivalent of LD_LIBRARY_PATH), to no avail. The only working solution is to...cd to directory where fftw3.dll is, but we definitely can't expect users to move around (and I also have opinions, not positive ones, about this behaviour of the loader). Also, we can't expect fftw and matlab libraries to be in the same directory

@t-young31
Copy link
Member

I've also just spent an hour or two on this and I am also stuck.

  1. I think moving the executable from build/Release to build/ would be a reasonable solution to combat the hilariously inconsistent behaviour
  2. Is more troubling.. I also found I needed to copy the .dlls into the same directory 😕 , which is not very user friendly. There also doesn't seem to be a way of using any kind of RPATH so I'm also unsure of the solution.

I suppose a static link to fftw and no matlab dependency would solve this, depending on our priorities we may want to leave this for a bit

@prmunro
Copy link
Collaborator

prmunro commented Jul 28, 2022

I'm not sure whether my review is required here or not. Are you asking whether to persist with this? I don't think I can offer a solution to this. I only ever compiled it under windows using Visual Studio. I understand why you're using cmake though.

@giordano
Copy link
Member Author

Note that we aren't having difficulties with compiling the program, but with running it: it can't find fftw and matlab shared libraries

Copy link
Collaborator

@prmunro prmunro left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this noting that at present the code won't run under windows. Can we add this as something to come back to?

@t-young31
Copy link
Member

Can we add this as something to come back to?

I'd be happy to do that


Comment for (potentially) the future: Alessandro mentioned on Friday it's apparently common to copy all the dependencies into the target directory. I also found this while googling dynamically linking on Windows. There is a CMake command in v3.21 to do just that but it requires SHARED libraries, and doesn't seem to work with the current CMake setup. With ${FFTW_ROOT} and ${Matlab_ROOT_DIR} it is possible to find the required dlls in a semi portable way, and copy them to the target directory. Ugly but maybe required – open to other better options!

@t-young31 t-young31 changed the title [CI] Run on macOS and Windows as well [CI] Run on Windows Aug 8, 2022
@t-young31
Copy link
Member

TDMS is now successfully building on GitHub actions under Windows! Turns out setting $PATH can work to find runtime .dlls (and is required because the MATLAB shared libraries have dependencies beyond just libmat.dll libmex.dll and libmx.dll). Due to some oddness with the zip file handling not all the system tests are run, but have instead been checked using an interactive session (following the CI build config/compile). Given this now closes #43 I'd be happy to merge this in and add an issue for running a useful system test on Windows CI, if required.

@t-young31 t-young31 mentioned this pull request Aug 9, 2022
@t-young31 t-young31 added the review required A review approval is required before merging label Aug 9, 2022
@t-young31 t-young31 removed the review required A review approval is required before merging label Aug 10, 2022
@t-young31 t-young31 merged commit c2b6e3b into main Aug 10, 2022
@t-young31 t-young31 deleted the mg/ci-windows-macos branch August 10, 2022 11:27
@t-young31 t-young31 restored the mg/ci-windows-macos branch August 10, 2022 11:29
@giordano giordano deleted the mg/ci-windows-macos branch August 22, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical Technical and meta issues, not related to physics but infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test compilation for Windows
4 participants