Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 msix/1.7 #6475
add msix/1.7 #6475
Changes from 17 commits
18957ae
d3388aa
418d594
0e77bd7
adc4aa2
9720eef
b082828
2290f0a
2fe4bea
b31d12a
b091f33
8d28d2c
8f88d74
f25ff8f
1d6d01a
206fea4
a3144aa
e050a90
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to always use external libs.
Using a an included/included vendored zlib will open the possibility of having some symbols available twice.
The linker can then choose symbols from msix or from zlib.
So please remove this option and always use external zlib (=our cci package)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option allows to choose system libraries for compression when building on some platforms:
If this option is
on
, thezlib
package from CCI will be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like current master changed the name of the
USE_EXTERNAL_ZLIB
variable already:https://github.com/microsoft/msix-packaging/blob/a8c86c68f18c7fb641fe5e60f31e86e176f9bd29/cmake/msix_options.cmake#L88-L94
Current master has:
https://github.com/microsoft/msix-packaging/blob/c8af99506ffd0c1513fad39cdadfac281723c3e3/cmake/msix_options.cmake#L16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upstream didn't rename the variable, we did it so that it reflects the idea that when it's
on
, the build will use the external (CCI) package forzlib
. The original name and description is:The modified version:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vendored libraries are often provided for users without package manager as a way to quickly get building without building all dependencies.
We're using conan here with the cci library. So I think it's best to remove this option and unconditionally disable using a vendored zlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the term "vendored" we meant libraries whose sources had been made a part of the library, when I wrote about system
zlib
I meant these ones:https://developer.android.com/ndk/guides/stable_apis#zlib_compression
https://developer.apple.com/documentation/compression
https://github.com/microsoft/msix-packaging/blob/3fbd28851b0ac525324781ec20634a2f0515c0ef/src/msix/CMakeLists.txt#L292_L298
We can omit these possibilities, shall we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we want to avoid (at all cost) is to use the vendored zlib here: https://github.com/microsoft/msix-packaging/blob/5f977a79d4f2bd189c1d99ae8f501316a282191a/lib/CMakeLists.txt#L22
Does the current combination of options + patches completely avoid that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current patch completely removes processing of the
lib
subdirectory:conan-center-index/recipes/msix/all/patches/1.7/cmake.patch
Line 79 in a3144aa
conan-center-index/cmake.patch at a3144aa · conan-io/conan-center-index
https://github.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As point out the patch delete the usage of this option so it can be removed from the recipe!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option hasn't been deleted, the commit just reverts it's name to the original one.