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.

Sign up for GitHub

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

Jump to bottom

CI:add MinGW builds #991

Merged
merged 3 commits into from
Jul 3, 2023
Merged

CI:add MinGW builds #991

merged 3 commits into from
Jul 3, 2023

Conversation

ccraluca
Copy link
Contributor

Add MinGW build and modify related files to add MinGW binaries to SWDownloads and release.

pcercuei
pcercuei suggested changes Jun 21, 2023
View reviewed changes
network.c Outdated Show resolved Hide resolved
CI/publish_deps.ps1 Show resolved Hide resolved
CI/publish_deps.ps1 Outdated Show resolved Hide resolved
CI/publish_deps.ps1 Outdated Show resolved Hide resolved
CI/build_win.ps1 Outdated Show resolved Hide resolved
@ccraluca ccraluca force-pushed the staging/add-mingw branch 2 times, most recently from 86015a3 to 937816c Compare June 21, 2023 20:27
rgetz
rgetz reviewed Jun 22, 2023
View reviewed changes
CI/azure/prepare_assets.sh Outdated Show resolved Hide resolved
@rgetz
Copy link
Contributor

rgetz commented Jun 22, 2023

the zip.txt describes one zip file, which will include all of mingw, VS 2019, VS2022 in separate directories.

In this archive, you should find the following directories:

When I look at the actual artifacts (and previous releases) - there are three separate zip files, that don't include the alternatives, and are not structured like the txt file indicates (no include dir).

Which is it?

nunojsa
nunojsa reviewed Jun 23, 2023
View reviewed changes
Copy link
Contributor

@nunojsa nunojsa left a comment

Choose a reason for hiding this comment

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

LGTM with the already proposed changes...

network.c Outdated Show resolved Hide resolved
@ccraluca
Copy link
Contributor Author

ccraluca commented Jun 23, 2023 •
edited
Loading

@rgetz, for these changes, I was following the latest version when we had MinGW builds (v0.21)
In the last 2 versions (v0.23 and v0.24) the structure was changed because we didn't have MinGW builds on Azure and now, because it is deployed, we switched to the old structure (aka a zip file containing directories for each Windows build, and an include directory containing the file iio.h)

@pcercuei
Copy link
Contributor

What is "argument 4"?

@pcercuei pcercuei requested review from pcercuei and rgetz June 23, 2023 08:45
@rgetz
Copy link
Contributor

rgetz commented Jun 23, 2023 •
edited
Loading

@rgetz, for these changes, I was following the latest version when we had MinGW builds (v0.21)

I'm not sure I understand - when I look at the Windows zip file for 0.21 it includes a directory structure that looks like:

└── libiio-0.21.g565bf68
    ├── include
    │   └── iio.h
    ├── MinGW32
    ├── MinGW64
    ├── MS32
    ├── MS64
    └── README.txt

I get, and agree with dropping 32-bit - no problem with that.

when I look at the artifacts from this PR I see three different and unique zip files (one for minGW, one for MS64 2019, one for MS64 MS2022).

There is no single zipfile which contains all.
The Windows-MinGW-W64.zip does not contain the README.txt file
The Windows-VS-2019-x64.zip does not contain the README.txt file
The Windows-VS-2022-x64.zip does not contain the README.txt file

@ccraluca
Copy link
Contributor Author

ccraluca commented Jun 23, 2023 •
edited
Loading

@rgetz, for these changes, I was following the latest version when we had MinGW builds (v0.21)

I'm not sure I understand - when I look at the Windows zip file for 0.21 it includes a directory structure that looks like:

└── libiio-0.21.g565bf68
    ├── include
    │   └── iio.h
    ├── MinGW32
    ├── MinGW64
    ├── MS32
    ├── MS64
    └── README.txt

I get, and agree with dropping 32-bit - no problem with that.

when I look at the artifacts from this PR I see three different and unique zip files (one for minGW, one for MS64 2019, one for MS64 MS2022).

There is no single zipfile which contains all. The Windows-MinGW-W64.zip does not contain the README.txt file The Windows-VS-2019-x64.zip does not contain the README.txt file The Windows-VS-2022-x64.zip does not contain the README.txt file

So if you are looking to CI/azure/prepare_assets.sh lines 34-47, there is defined a zip file that will have the structure described on README.txt, this is the one that will be for the GitHub release.
For SwDownloads we will have Windows-MinGW-W64-latest_master_libiio.zip, Windows-VS-2019-x64-latest_master_libiio.zip and Windows-VS-2022-x64-latest_master_libiio.zip (all these zip files will contain only files provided on Azure and no README.txt); see lines 81-86.
When you download artifacts from Azure (any artifact, not only for Windows) will be .zip because there are multiple files but this is not the structure used on GitHub release.

@pcercuei
Copy link
Contributor

@ccraluca, the snprintf() format fix is actually invalid. On Linux x86_64, sin6_scope_id is 32-bit, while "unsigned long" is 64-bit.

Knowing that on Windows "long" is always 32-bit, then we can deduce that this field will always be 32-bit, and therefore the best way to address the warning is to use %u with a cast to (unsigned int).

@rgetz
Copy link
Contributor

rgetz commented Jun 23, 2023 •
edited
Loading

CI/azure/prepare_assets.sh

Thanks - that's what I was missing.

Is there a way in the PR to see the resulting file, so we can give some feedback on it, or we need to do a pre-release?

@ccraluca
Copy link
Contributor Author

CI/azure/prepare_assets.sh

Thanks - that's what I was missing.

Is there a way in the PR to see the resulting file, so we can give some feedback on it, or we need to do a pre-release?

Yes, a pre-release or if it's ok to push a test tag, that way we would check before merging to master.

@rgetz
Copy link
Contributor

rgetz commented Jun 26, 2023

Yes, a pre-release or if it's ok to push a test tag, that way we would check before merging to master.

Either way - what ever is easier for you.

However - you need to address @pcercuei 's flag first.

the snprintf() format fix is actually invalid.

@ccraluca ccraluca force-pushed the staging/add-mingw branch 2 times, most recently from f757ae2 to 8c814b1 Compare June 26, 2023 18:14
@ccraluca
Copy link
Contributor Author

@ccraluca, the snprintf() format fix is actually invalid. On Linux x86_64, sin6_scope_id is 32-bit, while "unsigned long" is 64-bit.

Knowing that on Windows "long" is always 32-bit, then we can deduce that this field will always be 32-bit, and therefore the best way to address the warning is to use %u with a cast to (unsigned int).

Modification added.

@ccraluca
Copy link
Contributor Author

Yes, a pre-release or if it's ok to push a test tag, that way we would check before merging to master.

Either way - what ever is easier for you.

However - you need to address @pcercuei 's flag first.

the snprintf() format fix is actually invalid.

A test-tag was pushed and artifacts can be checked. Once everything is ok, I will need to modify conditions on YAML file back for v-tags.

@ccraluca ccraluca force-pushed the staging/add-mingw branch 4 times, most recently from 68bb90b to 351da8d Compare June 27, 2023 12:12
@ccraluca
Copy link
Contributor Author

Fixed the structure of Windows.zip on GitHub release

rgetz
rgetz approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@rgetz rgetz left a comment

Choose a reason for hiding this comment

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

Looks great now - thanks for the effort.

network.c Outdated Show resolved Hide resolved
CI/azure/prepare_assets.sh Outdated Show resolved Hide resolved
@tfcollins tfcollins force-pushed the staging/add-mingw branch 16 times, most recently from 213fc80 to 9cbd142 Compare June 28, 2023 00:32
pcercuei
pcercuei reviewed Jul 3, 2023
View reviewed changes
CI/azure/macos_tar_fixup.sh Outdated
rm "../${tarname}"
tar -czf "../${tarname}" .
cd ..
rm -rf temp
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing end-of-line character

ccraluca and others added 3 commits July 3, 2023 11:37
@ccraluca @pcercuei
network: fix snprintf format string for MinGW
74b4c0b 
MinGW build fail on unsigned int argument because long unsigned
int is expected.

Signed-off-by: Raluca Groza <raluca.groza@analog.com>
@ccraluca @pcercuei
CI:add MinGW build
315be8e 
Add MinGW build and modify related files to add artifacts to
SwDownloads and GitHub release.

Signed-off-by: Raluca Groza <raluca.groza@analog.com>
@tfcollins @pcercuei
Add rpathing and libusb updates for macOS CI
af19492 
Signed-off-by: Travis F. Collins <travis.collins@analog.com>
@pcercuei pcercuei self-requested a review July 3, 2023 10:10
@pcercuei pcercuei merged commit 4b71f07 into master Jul 3, 2023
21 checks passed
@pcercuei pcercuei deleted the staging/add-mingw branch July 3, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgetz rgetz rgetz approved these changes

@nunojsa nunojsa nunojsa left review comments

@pcercuei pcercuei pcercuei approved these changes

@tfcollins tfcollins Awaiting requested review from tfcollins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ccraluca @rgetz @pcercuei @nunojsa @tfcollins