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

upgrade zlib version to 1.2.13, which is the latest release. #177

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jackie9527
Copy link
Contributor

@Jackie9527 Jackie9527 commented Feb 6, 2023

Signed-off-by: Jackie9527 <80555200+Jackie9527@users.noreply.github.com>
@nick-l-o3de
Copy link
Contributor

nick-l-o3de commented Feb 20, 2023

one thing to caution here, zlib is so base and fundamental to everything that this will potentially cascade to needing to then update many other packages.

Unless something really serious forces us to change zlib, I'd highly recommend against it. Was it a security patch or something?

The situation here is that we distribute it as a static lib, so upgrading the package here will deliver a new static lib, and everything linking to that static lib will now link to the new one (yay!) So you don't need to update other packages even if they depend on zlib - as long as those other packages also deliver as static libs.

However, if there are packages that deliver as dlls or executables, and those packages will pack the contents of the stale zlib into their own dlls and will have older versions even after we ship this.

If we're doing this for some new feature it probably doesn't matter. if we're doing this to solve a real security hole, then we'd need to also issue new versions of the dll or exe distributed libraries in 3p that ingest the old zlib.

  • consider updating the spdx license files at the root of the repo, too

the packages I'm aware of that may use and ingest our zlib and ship it as a non-static link would be

  • AWS Native SDK (possibly? does it come as lib files or dlls?)
  • ASsimp (same question, not sure)
  • openimageio/opencolorio (which ingest zlib)
  • potentially qt (it ships as dlls)

There may also be others. It really depends on if its a serious security patch that would affect those or if its just new features or somehting.

Copy link
Contributor

@nick-l-o3de nick-l-o3de left a comment

Choose a reason for hiding this comment

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

that being said, there's nothing I spy wrong with this.
Its a pity the thing was one of the first packages ever made and didn't have any self tests tho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants