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

Fix Windows Builds #1183

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

Fix Windows Builds #1183

wants to merge 2 commits into from

Conversation

cristina-suteu
Copy link
Collaborator

PR Description

As things was before, for the windows deps an archive was downloaded from swdownloads. Said archive contains windows deps compiled with MinGW. The installer is created based on the MSVC builds. Both MSVC and MinGW builds were using deps from this archive compiled with MinGW. While the project seemed to be building okay, this caused problems which were visible in both installer and MSVC libiio zips at runtime.
By adding a new archive with deps compiled with MSVC to be used when building libiio with MSVC, the issue is resolved.
Moreover, a check for the compiler used was added in the windows build script. This ensures the correct paths to libiio's deps according to the compiler used.
This fixes issue #1174

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have commented new code, particularly complex or unclear areas
  • I have checked that I did not introduced new warnings or errors (CI output)
  • I have checked that components that use libiio did not get broken
  • I have updated the documentation accordingly (GitHub Pages, READMEs, etc)

previously, deps compiled with mingw were used to build for MSVC
added a new archive with deps compiled for MSVC to be used in this case
added condition in build_win script to check for compiler and updated cmake paths for MSVC case

Signed-off-by: Cristina Suteu <[email protected]>
extract and copy MSVC deps to BUILD_ARTIFACTSTAGINGDIRECTORY
update name of libserialport.dll in iss file

Signed-off-by: Cristina Suteu <[email protected]>
@@ -9,7 +9,10 @@ cd $src_dir
mkdir dependencies
cd dependencies
wget http://swdownloads.analog.com/cse/build/libiio-deps-20220517.zip -OutFile "libiio-win-deps.zip"
wget https://swdownloads.analog.com/cse/build/libiio-deps-20240627.zip -OutFile "libiio-msvc-deps.zip"
Copy link
Contributor

Choose a reason for hiding this comment

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

It would help (in the future) to rename libiio-deps-20200627 to a name that doesn't change over time. For example: libiio-deps-msvc. If at some point in time, there will be a new libiio-deps-yyyymmdd ,then there will be only one place to update the name. Now there are extra 4 places besides this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i used the yyyy-mm-dd format, to match the naming used in the other archive.
as a side note, the old archive with mingw compiled deps should also be updated to contain all required files. would it make sense to update the names of both archives to leave out the yyyy-mm-dd suffix after fixing the mingw archive as well ?

@rgetz
Copy link
Contributor

rgetz commented Jul 13, 2024

As things was before - was sort of fragile and is sort of iffy from a licensing standpoint...

replicating it might not be the right path forward.

@pamolloy pamolloy mentioned this pull request Jul 18, 2024
8 tasks
@cristina-suteu
Copy link
Collaborator Author

As things was before - was sort of fragile and is sort of iffy from a licensing standpoint...

replicating it might not be the right path forward.

I agree. We should provide the complete build recipe for deps and libiio itself. The purpose of this PR is to fix the issue which was reported. This is more of a hot-fix, than a long term solution, so people can download and use the windows installers. If this is not that urgent, I can close this and address this issue and the entire build recipe in a different PR.
@mhennerich , @dNechita , @tfcollins , what do you think ?

@tfcollins
Copy link
Contributor

Providing the complete build would address the licensing issue. The installer probably needs to be checked that it does reference anything packaged as well

@AAndrisa AAndrisa mentioned this pull request Sep 12, 2024
8 tasks
@rgetz
Copy link
Contributor

rgetz commented Sep 13, 2024

I think this is replaced by #1197 and this can be closed (without merging)?

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