Skip to content

Conditionally update zlib name in get_system_library_name#662

Merged
dstoup merged 1 commit intomasterfrom
fix-static-zlib-target
Apr 12, 2021
Merged

Conditionally update zlib name in get_system_library_name#662
dstoup merged 1 commit intomasterfrom
fix-static-zlib-target

Conversation

@as6520
Copy link
Copy Markdown
Contributor

@as6520 as6520 commented Apr 8, 2021

The static build for zlib is called libz.a rather libzlib.a. Thus PR checks if the build is static or dynamic and provides the zlib name based on the type of build

@as6520 as6520 requested review from dstoup and johnwparent April 8, 2021 14:23
Copy link
Copy Markdown
Collaborator

@dstoup dstoup 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 ok with this provided CI passes. I have to check on the static CI to see if it's running. That said, as long as it doesn't mess up what most people do, all good. We can fix other issues later. At some point I need to overhaul how things are done in Fletch but that's not today.

@kwcvrobot
Copy link
Copy Markdown
Collaborator

@kwcvrobot
Copy link
Copy Markdown
Collaborator

@kwcvrobot
Copy link
Copy Markdown
Collaborator

@kwcvrobot
Copy link
Copy Markdown
Collaborator

@johnwparent
Copy link
Copy Markdown
Member

Looks like Windows failure is due to the Kwiver cmake discover tests issue, rather than a bug in this PR.

@dstoup
Copy link
Copy Markdown
Collaborator

dstoup commented Apr 12, 2021

Looks good. Do we want/need this patch for the release? If so, change the target to the release branch, otherwise let me know and I will merge.

@as6520
Copy link
Copy Markdown
Contributor Author

as6520 commented Apr 12, 2021

I think the fix is required for the kwiver wheel and does not effect the shared build. So it is not necessary to add it to 1.4.x.

@dstoup dstoup merged commit a525706 into master Apr 12, 2021
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