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

Set metadata in SRT DLL on Windows #639

Merged
merged 53 commits into from Nov 4, 2019
Merged

Conversation

lewk2
Copy link
Contributor

@lewk2 lewk2 commented Apr 11, 2019

This PR shows changes to automated metadata against the SRT DLL created during CI builds.

Unfortunately, it builds upon the Windows build changes in another PR, so it looks a little more cluttered until that is merged. However, this PR allows inspection of the proposed change.

The key file is srt_shared.rc, which is used during compilation to set the metadata. CMakeLists is changed to pick up the build number from CI and pass into the version header used at compile to ident the binaries.

The windows DLL is also marked as 'delay load' in this PR, which means it will run happily without any Open SSL dependencies being available (calls to dependent functions will obviously fail) - which is useful if someone has restrictions on making Open SSL available in their usage.

Probably this PR should be revised regarding the 'copyright' name - I have no objection to it just being set to Haivision, but maybe others might - it's not my call. But it's probably better to try and face this sooner rather than later... however, i'm also perfectly fine with that field being deleted rather than it hold up the PR! Copyright value is now just empty, because empty is not a change and therefore not controversial. I don't want to hold up this PR by having some long debate about what the value should be - I defer setting this to someone from Haivision!

This PR now also automates the AppVeyor build number, drawing the source values from the master value set centrally. It also marks the resulting DLLs with the git repo name, along with the branch / commit numbers in the 'File Description' property of the DLL - allowing easy tracing of origin - see here:

image

It might make sense to also close this PR once reviewed and then re-issue when PR614 gets merged.

lewk2 added 14 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
-srt static library DLL on windows now has file version and copyright metadata set
@maxsharabayko
Copy link
Collaborator

Please resolve Travis CI build errors. Compiler: x86_64-w64-mingw32-g++ C++

[ 75%] Building RC object CMakeFiles/srt_shared.dir/srtcore/srt_shared.rc.obj
make[2]: windres: Command not found
make[2]: *** [CMakeFiles/srt_shared.dir/srtcore/srt_shared.rc.obj] Error 127
make[1]: *** [CMakeFiles/srt_shared.dir/all] Error 2
make: *** [all] Error 2
The command "make" exited with 2.

@lewk2
Copy link
Contributor Author

lewk2 commented Sep 3, 2019

Sorry I took so long to come back to this. I have now merged master (resolves conflicts) and put in the small tweak to stop any of the Windows metadata being assigned during CYGWIN builds.
PR614 has been kicked down the road a few times - this PR continues to be blended between the two of these branches... I would still recommend that PR614 gets merged and then this re-issued to prevent the mixing of the changes.

@maxsharabayko maxsharabayko added this to Needs review in Development via automation Oct 24, 2019
@maxsharabayko maxsharabayko added the [build] Area: Changes in build files label Oct 24, 2019
@maxsharabayko maxsharabayko added this to the v1.4.1 milestone Oct 24, 2019
Development automation moved this from Needs review to Reviewer approved Oct 24, 2019
CMakeLists.txt Outdated Show resolved Hide resolved
srtcore/filelist.maf Outdated Show resolved Hide resolved
Development automation moved this from Reviewer approved to Needs review Oct 24, 2019
@lewk2 lewk2 requested a review from ethouris October 24, 2019 13:25
Development automation moved this from Needs review to Reviewer approved Oct 25, 2019
@ethouris ethouris added Status: Completed Type: Maintenance Work required to maintain or clean up the code labels Oct 25, 2019
Development automation moved this from Reviewer approved to Needs review Oct 28, 2019
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 remove (again) googletest submodule, it was re-added in commit 5208f83. In the future, please consider re-organizing your PRs into meaningful commits with interactive rebase or, otherwise, squashing them into a single commit. Thanks!

@lewk2
Copy link
Contributor Author

lewk2 commented Nov 4, 2019

Please remove (again) googletest submodule, it was re-added in commit 5208f83. In the future, please consider re-organizing your PRs into meaningful commits with interactive rebase or, otherwise, squashing them into a single commit. Thanks!

This is fixed - I'm in contact with @maxlovic about a separate PR that will then move the build to a sub-folder in the build, and then the .gitignore can cover this.

I have also learned a lot about workflows for PRs, and will try to avoid such a long, messy trail in the future.

@lewk2 lewk2 closed this Nov 4, 2019
Development automation moved this from Needs review to Done Nov 4, 2019
@lewk2 lewk2 reopened this Nov 4, 2019
Development automation moved this from Done to In progress Nov 4, 2019
@lewk2 lewk2 requested a review from rndi November 4, 2019 10:44
@rndi rndi merged commit 291bdf2 into Haivision:master Nov 4, 2019
Development automation moved this from In progress to Done Nov 4, 2019
@lewk2 lewk2 deleted the Win_Build_Option branch November 5, 2019 16:11
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 Type: Maintenance Work required to maintain or clean up the code
Projects
No open projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants