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

Add 32-bit Windows and consume Pthreads from NuGet #614

Merged
merged 39 commits into from
Oct 10, 2019

Conversation

lewk2
Copy link
Contributor

@lewk2 lewk2 commented Mar 18, 2019

This PR contains two specific enhancements:

  • The addition of 32-bit builds to the CI system on VS2015, along with PDB collection for debug builds.
  • A move of pthreads away from compile-from-source each time, to instead use a pthreads-win{bitness} package hosted on NuGet, compiled from source via a Cinegy fork of the original package.

It also includes various small cleanups within the build system for consistency and clarity (e.g. consistent use of quotes). It also drops VS2013 from CI, since with the inclusion of 32-bit configurations caused a doubling of build time.

lewk2 added 6 commits March 14, 2019 20:06
* -add x86 compile to appveyor build for win

* -tune the way VS SLN gets platform value

* -make use of quotes consistent in appveyor

* -factor bitness into folder names for libs

* -add 64bit hint for pthread_include_dir

* -add bitness to artifact name

* -add VS_VERSION to artifact name

* -gather PDB for debug builds

* -set more sensible build number in appveyor
-remove vs2013 hangovers
-add .gitingore extras
-stop pthreads going into built package zip
.gitignore Outdated Show resolved Hide resolved
nuget.config Show resolved Hide resolved
@lewk2
Copy link
Contributor Author

lewk2 commented Mar 26, 2019 via email

CMakeLists.txt Outdated Show resolved Hide resolved
@MathieuPOUX
Copy link
Contributor

I appreciate these change proposition which removes the PThreadWIN32 external dependency on Win (If I understand correctly). It makes SRT lib more easy to embed in a cross-platform project!

@lewk2
Copy link
Contributor Author

lewk2 commented Mar 28, 2019

I appreciate these change proposition which removes the PThreadWIN32 external dependency on Win (If I understand correctly). It makes SRT lib more easy to embed in a cross-platform project!

This really does not remove the dependency from runtime - it just removes the need to repeatedly build and rebuild the lib file for pthreads components, since the lib is just grabbed from NuGet instead.

It therefore makes the build both quicker on the CI server, and easier for people to set up local workstation builds.

@MathieuPOUX
Copy link
Contributor

I appreciate these change proposition which removes the PThreadWIN32 external dependency on Win (If I understand correctly). It makes SRT lib more easy to embed in a cross-platform project!

This really does not remove the dependency from runtime - it just removes the need to repeatedly build and rebuild the lib file for pthreads components, since the lib is just grabbed from NuGet instead.

It therefore makes the build both quicker on the CI server, and easier for people to set up local workstation builds.

Ok I see! thanks for details

@maxsharabayko maxsharabayko added the [build] Area: Changes in build files label Apr 11, 2019
@maxsharabayko maxsharabayko added this to the v.1.3.3 milestone Apr 11, 2019
@maxsharabayko maxsharabayko modified the milestones: v.1.3.3, v.1.3.4 May 29, 2019
@maxsharabayko maxsharabayko modified the milestones: v1.3.4, v1.4.1 Aug 9, 2019
.appveyor.yml Outdated Show resolved Hide resolved
.appveyor.yml Show resolved Hide resolved
.appveyor.yml Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@lewk2 lewk2 requested a review from maxsharabayko September 18, 2019 11:43
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

Please resolve conflict in .gitignore

CMakeLists.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scripts/gather-package.bat Outdated Show resolved Hide resolved
srtcore/filelist_win32_shared.maf Outdated Show resolved Hide resolved
@lewk2 lewk2 changed the title Add 32-bit Windows and consume Pthreads from NuGet Add 32-bit Windows and consume Pthreads from NuGet, set windows DLL to delay mode with metadata Oct 8, 2019
srtcore/filelist.maf Outdated Show resolved Hide resolved
@maxsharabayko maxsharabayko changed the title Add 32-bit Windows and consume Pthreads from NuGet, set windows DLL to delay mode with metadata Add 32-bit Windows and consume Pthreads from NuGet Oct 8, 2019
Copy link
Contributor Author

@lewk2 lewk2 left a comment

Choose a reason for hiding this comment

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

all done - seems windows git does some magic eating that last line

srtcore/filelist.maf Outdated Show resolved Hide resolved
Copy link
Collaborator

@rndi rndi left a comment

Choose a reason for hiding this comment

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

Please let me know if you will be flattening history on this PR to some meaningful commits. Otherwise, I can squash all history into a single commit.

.appveyor.yml Outdated Show resolved Hide resolved
scripts/gather-package.bat Show resolved Hide resolved
@maxsharabayko
Copy link
Collaborator

Please let me know if you will be flattening history on this PR to some meaningful commits. Otherwise, I can squash all history into a single commit.

@rndi This PR used to have changes from PR #639. Now it does not. It can be squash-merged into one commit.

This will return, but scripted, with metadata settings
@rndi rndi merged commit 0e36bf2 into Haivision:master Oct 10, 2019
@lewk2 lewk2 deleted the Branch_v1.3.2 branch October 23, 2019 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[build] Area: Changes in build files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants