add msix/1.7 #6475

vaerizk · 2021-07-23T18:04:44Z

msix/1.7

I've read the guidelines for contributing.
I've followed the PEP8 style guides for Python code in the recipes.
I've used the latest Conan client version.
I've tried at least one configuration locally with the
conan-center hook activated.

vaerizk · 2021-07-23T18:23:26Z

Is there a possibility to use a dependency with non-default value for some options? My build failed because we need char_type=char16_t for xerces-c and the default value is different.

recipes/msix/all/conanfile.py

recipes/msix/all/conandata.yml

uilianries · 2021-07-23T21:05:59Z

@SSE4 @uilianries @madebr

Is there a possibility to use a dependency with non-default value for some options? My build failed because we need char_type=char16_t for xerces-c and the default value is different.

Unfortunately not. What you can do is changing the default option if it's invalid and cause all packages invalid.

Please, follow @Croydon review.

madebr · 2021-07-23T23:57:04Z

recipes/msix/all/conanfile.py

+        "crypto_lib": ["crypt32", "openssl"],
+        "pack": [True, False],
+        "skip_bundles": [True, False],
+        "use_external_zlib": [True, False],


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)

This option allows to choose system libraries for compression when building on some platforms:

set(COMPRESSION_LIB "zlib") if(((IOS) OR (MACOS)) AND (NOT USE_EXTERNAL_ZLIB)) set(COMPRESSION_LIB "libCompression") elseif((AOSP) AND (NOT USE_EXTERNAL_ZLIB)) set(COMPRESSION_LIB "inbox zlib") endif()

If this option is on, the zlib package from CCI will be used.

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

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 for zlib. The original name and description is:

option(USE_MSIX_SDK_ZLIB "Use zlib implementation under lib/zlib. If off, uses inbox compression library. For Windows and Linux this is no-opt." OFF)

The modified version:

option(USE_EXTERNAL_ZLIB "Use an external zlib package. If off, uses inbox compression library. For Windows and Linux this is no-opt." OFF)

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.

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?

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?

Does the current combination of options + patches completely avoid that?

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

-add_subdirectory(lib)

conan-center-index/cmake.patch at a3144aa · conan-io/conan-center-index
https://github.com

As point out the patch delete the usage of this option so it can be removed from the recipe!

Suggested change

"use_external_zlib": [True, False],

The option hasn't been deleted, the commit just reverts it's name to the original one.

recipes/msix/all/conanfile.py

recipes/msix/all/test_package/example.cpp

vaerizk · 2021-07-26T14:20:31Z

Unfortunately not. What you can do is changing the default option if it's invalid and cause all packages invalid.

@uilianries What do you mean by "cause all packages invalid"? Are you talking about the case when the default option value doesn't satisfy any dependent package?

According to xerces-c cmake-files, for any compiler that supports char16_t this type will be chosen as the default for char_type, so we could choose char16_t as the default value. But there are two packages in CCI that depend on xerces-c atm: quickfast and gdal. If we change the default for char_type in xerces-c, it may break the build for these libraries.

vaerizk · 2021-07-26T15:04:39Z

Accidentally closed.

- polish the `source` method

- add required conan version

- fix the `crypto_lib` option

- add `msxml6` to the list of system libraries

- move configuration checks to `validate()`

recipes/msix/all/conanfile.py

recipes/msix/all/patches/1.7/cmake.patch

- restrict the package

vaerizk · 2021-08-11T18:33:05Z

@jgsogo

About missing binaries:

openssl/1.0.2t is not available for Macos+armv8, it fails: https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/TapaholesList/233/summary.json (read here about this new configuration: [changelog] Enable M1 builds (and more things) #6658)

xerces-c: only packages with default options are available, as said in some review.

done

jgsogo · 2021-08-12T07:29:25Z

@jgsogo

About missing binaries:

openssl/1.0.2t is not available for Macos+armv8, it fails: https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/TapaholesList/233/summary.json (read here about this new configuration: [changelog] Enable M1 builds (and more things) #6658)

xerces-c: only packages with default options are available, as said in some review.

done

Hi! Sorry, maybe I did assume some implicit knowledge of how CCI works and I didn't explain myself. People are contributing to ConanCenter continuously, everyone is doing a great job, but recipes evolve and things might be broken from time to time. We try to think about ConanCenter as a whole and sometimes some PRs need to wait for other issues to be fixed and maximize the value for a given effort.

In this case, you shouldn't prevent your recipe from using openssl in Macos, it was just a request to fix that recipe and someone did it: #6588. So now, you should remove that ConanInvalidConfiguration because it is no longer a missing package. 😃

About the xerces-c issue. Here in CCI we are only building packages with the default options (for many different configurations), it is just because we cannot afford to build all the combinations.... but I'm sure that this recipe would work with other options rather than the default ones. Users at home might build it from sources so the recipe shouldn't block them. Here:

        # the current CCI xerces-c recipe is not supported
        if (self.options.xml_parser == "xerces" and
            self.options["xerces-c"].char_type != "char16_t"):
                raise ConanInvalidConfiguration("Only char16_t is supported for xerces-c")

this recipe will block those users that are building from sources. If that combination of requirements and options work, don't raise the exception, someone locally might need to use it.

jgsogo

(Following previous comment)

recipes/msix/all/conanfile.py

- polish the recipe

- remove the restriction for openssl

- add cxx standard check

- reduce the patch size

recipes/msix/all/patches/1.7/cmake.patch

- revert the `USE_MSIX_SDK_ZLIB` option renaming

conan-center-bot · 2021-08-23T19:56:14Z

All green in build 10 (e050a90f69494cd24755a65778701df358a08588):

msix/1.7@:
All packages built successfully! (All logs)

prince-chrismc

Two questions

recipes/msix/all/conanfile.py

prince-chrismc · 2021-09-26T01:44:23Z

recipes/msix/all/patches/1.7/cmake.patch

+         list(APPEND MsixSrc PAL/Applicability/Win32/Applicability.cpp)
+     elseif(LINUX)
+-        find_package(ICU REQUIRED COMPONENTS uc)
+        target_link_libraries(${PROJECT_NAME} PRIVATE CONAN_PKG::icu)


Conan V2.0 wont have this namespace target

I think this issue should be addressed when Conan 2.0 is released, or can you suggest any solution?

Does not make sense to merge something we know will break... we are making an active effort to make the migration easier

🤔 You can keep the call to find package and use the Conan global targets... here could be icu::icu

@prince-chrismc I think we will block the usage of CONAN_PKG, CONAN_LIBS and similar in hook soon

That's the best way to fix all the recipes

This namespace is provided by the cmake generator and as far as I know this generator is the only way to handle cmake settings like MT/MD passing, so right now the only way to replace this namespace is to use, for example, the cmake_find_package generator along with cmake. But if the cmake generator is removed in Conan 2.0, the recipe will require fix anyway, so does it make sense to add another generator and replace the namespace at the moment?

Well the new generators will be close to seamless for the cmake_find_package_multi espcisially around the injection of targets. If we stick if the original intended use of the build script it will be more future proof?

and yes there will be changes but it's not obvious what that will be... I know the CCI team has experimented with the new workflow but I personally have not so I am not sure... I have the expectation the cmake wrapper will survive

no, CMake wrapper is not expected to survive. all the things conan_basic_setup did previously now should be set inside the CMake toolchain file generated by conan.
it should enable more seamless and less intrusive integration.
for CCI recipes, it means few simplifications:

removal of wrapping CMakeLists.txt if any

removal of any patches/replace_in_files doing CMAKE_SOURCE_DIR/CMAKE_CURRENT_SOURCE_DIR adjustments - now recipe doesn't change project root

removal of any patches/replace_in_files doing insertions of conan_basic_setup, shouldn't be necessary

probably, it will have more implications on recipes, but at least these should be welcomed changes IMO
the workflow will change only with CMakeDeps/CMakeToolchain generators, for now you can keep doing that you doing

recipes/msix/all/conanfile.py

jgsogo

We really prefer to use real targets instead of CONAN_PKG:: ones, but as long as the CI is happy (no hook checking it) this is not a blocker. There will be time to adapt all the recipes.

add msix/1.7

18957ae

This comment has been minimized.

Sign in to view

conan-center-bot added the Failed label Jul 23, 2021

Croydon reviewed Jul 23, 2021

View reviewed changes

recipes/msix/all/conanfile.py Outdated Show resolved Hide resolved

Croydon reviewed Jul 23, 2021

View reviewed changes

recipes/msix/all/conanfile.py Outdated Show resolved Hide resolved

Croydon reviewed Jul 23, 2021

View reviewed changes

recipes/msix/all/conanfile.py Show resolved Hide resolved

Croydon reviewed Jul 23, 2021

View reviewed changes

recipes/msix/all/conandata.yml Show resolved Hide resolved

madebr reviewed Jul 23, 2021

View reviewed changes

recipes/msix/all/conanfile.py Show resolved Hide resolved

madebr reviewed Jul 23, 2021

View reviewed changes

recipes/msix/all/conanfile.py Outdated Show resolved Hide resolved

madebr reviewed Jul 24, 2021

View reviewed changes

recipes/msix/all/conanfile.py Show resolved Hide resolved

madebr reviewed Jul 24, 2021

View reviewed changes

recipes/msix/all/conanfile.py Show resolved Hide resolved

madebr reviewed Jul 24, 2021

View reviewed changes

recipes/msix/all/conanfile.py Outdated Show resolved Hide resolved

madebr reviewed Jul 24, 2021

View reviewed changes

recipes/msix/all/conanfile.py Show resolved Hide resolved

madebr reviewed Jul 24, 2021

View reviewed changes

recipes/msix/all/conanfile.py Show resolved Hide resolved

madebr reviewed Jul 24, 2021

View reviewed changes

recipes/msix/all/test_package/example.cpp Show resolved Hide resolved

vaerizk closed this Jul 26, 2021

vaerizk reopened this Jul 26, 2021

This comment has been minimized.

Sign in to view

Croydon and others added 4 commits July 27, 2021 20:05

add msix/1.7

d3388aa

- polish the `source` method

add msix/1.7

418d594

- add required conan version

add msix/1.7

0e77bd7

- fix the `crypto_lib` option

add msix/1.7

adc4aa2

- add `msxml6` to the list of system libraries

This comment has been minimized.

Sign in to view

add msix/1.7

9720eef

- move configuration checks to `validate()`

madebr reviewed Jul 30, 2021

View reviewed changes

recipes/msix/all/conanfile.py Outdated Show resolved Hide resolved

madebr reviewed Aug 9, 2021

View reviewed changes

recipes/msix/all/patches/1.7/cmake.patch Outdated Show resolved Hide resolved

add msix/1.7

8f88d74

- restrict the package

This comment has been minimized.

Sign in to view

jgsogo reviewed Aug 12, 2021

View reviewed changes

recipes/msix/all/conanfile.py Outdated Show resolved Hide resolved

recipes/msix/all/conanfile.py Outdated Show resolved Hide resolved

vaerizk added 3 commits August 13, 2021 20:25

add msix/1.7

f25ff8f

- polish the recipe

add msix/1.7

1d6d01a

- remove the restriction for openssl

add msix/1.7

206fea4

- add cxx standard check

This comment has been minimized.

Sign in to view

conan-center-bot removed the Failed label Aug 16, 2021

add msix/1.7

a3144aa

- reduce the patch size

vaerizk requested a review from jgsogo August 20, 2021 19:42

This comment has been minimized.

Sign in to view

madebr reviewed Aug 20, 2021

View reviewed changes

recipes/msix/all/patches/1.7/cmake.patch Outdated Show resolved Hide resolved

add msix/1.7

e050a90

- revert the `USE_MSIX_SDK_ZLIB` option renaming

conan-center-bot requested review from SSE4 and uilianries August 27, 2021 11:59

conan-center-bot requested review from czoido and danimtb September 16, 2021 11:59

SSE4 approved these changes Sep 25, 2021

View reviewed changes

prince-chrismc mentioned this pull request Sep 25, 2021

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

prince-chrismc reviewed Sep 26, 2021

View reviewed changes

recipes/msix/all/conanfile.py Show resolved Hide resolved

prince-chrismc approved these changes Sep 27, 2021

View reviewed changes

conan-center-bot requested a review from memsharded September 29, 2021 11:58

jgsogo approved these changes Sep 29, 2021

View reviewed changes

conan-center-bot merged commit a57393b into conan-io:master Sep 29, 2021

vaerizk mentioned this pull request Nov 25, 2021

[request] some of most relevant c++ libraries in github (by number of stars) #621

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add msix/1.7 #6475

add msix/1.7 #6475

vaerizk commented Jul 23, 2021

This comment has been minimized.

vaerizk commented Jul 23, 2021

uilianries commented Jul 23, 2021

madebr Jul 23, 2021

vaerizk Jul 27, 2021

madebr Jul 27, 2021

vaerizk Jul 30, 2021

madebr Aug 9, 2021

vaerizk Aug 12, 2021

madebr Aug 20, 2021

Nipheris Aug 23, 2021

prince-chrismc Sep 26, 2021

vaerizk Sep 27, 2021

vaerizk commented Jul 26, 2021 •

edited

Loading

vaerizk commented Jul 26, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vaerizk commented Aug 11, 2021

jgsogo commented Aug 12, 2021

jgsogo left a comment

This comment has been minimized.

This comment has been minimized.

conan-center-bot commented Aug 23, 2021

prince-chrismc left a comment

prince-chrismc Sep 26, 2021

vaerizk Sep 27, 2021

prince-chrismc Sep 27, 2021

SSE4 Sep 27, 2021

prince-chrismc Sep 27, 2021

vaerizk Sep 27, 2021

prince-chrismc Sep 27, 2021

SSE4 Sep 27, 2021

jgsogo left a comment

add msix/1.7 #6475

add msix/1.7 #6475

Conversation

vaerizk commented Jul 23, 2021

This comment has been minimized.

vaerizk commented Jul 23, 2021

uilianries commented Jul 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaerizk commented Jul 26, 2021 • edited Loading

vaerizk commented Jul 26, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vaerizk commented Aug 11, 2021

jgsogo commented Aug 12, 2021

jgsogo left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

conan-center-bot commented Aug 23, 2021

prince-chrismc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo left a comment

Choose a reason for hiding this comment

vaerizk commented Jul 26, 2021 •

edited

Loading