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

wasmtime: need to download binaries in build #7190

Merged
merged 6 commits into from
Sep 9, 2021

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Sep 7, 2021

Specify library name and version: wasmtime/all

wasmtime is packaging binaries, so we need to download the binaries in the build method.

@redradist
This is an important fix for wasmtime.
I also could remove the use of CONAN_COMPILE_DEFINITIONS_WASMTIME in the test package.


  • 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.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@madebr madebr mentioned this pull request Sep 7, 2021
4 tasks
@conan-center-bot

This comment has been minimized.

@SSE4 SSE4 requested a review from uilianries September 7, 2021 15:44
@@ -1,13 +1,11 @@
cmake_minimum_required(VERSION 3.1)
project(PackageTest C)

set(CMAKE_C_STANDARD 11)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to use set(CMAKE_C_STANDARD 11) at top of file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an idiom we adopted at cci.
"Modern CMake" is all about using targets and avoiding global variables.

It probably does not matter for such a small example, but most of us believe it's better to use best practices as much as possible.

tools.get(**self.conan_data["sources"][self.version][self._sources_key][str(self.settings.arch)], destination=self.source_folder, strip_root=True)
def package_id(self):
del self.info.settings.compiler.version
if self.settings.compiler == "clang":
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like it is dirty hack ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a way to alias the packages of gcc and clang.
I couldn't remove self.info.settings.compiler.

I argue it's a clean hack 😛

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done it this way to avoid creating duplicate packages.
Packages with compiler=gcc and compiler=clang will be identical, because nothing is built.

If I understand compatible compiler correctly, then a package for gcc and clang will be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually used a custom package id: see https://docs.conan.io/en/latest/creating_packages/define_abi_compatibility.html#defining-a-custom-package-id

So not a hack after all. It's documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compatible packages would require me to list all gcc/clang versions that are compatible which does not exist, or will be always incomplete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compatible compilers works, but it will cause c3i to generate a package for gcc and clang, which are the same.
So I'd prefer to keep the package_id method as it is now.

Copy link
Member

Choose a reason for hiding this comment

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

Right, it will generate for both clang and gcc. Indeed using same pkg id will save cci resources.

@conan-center-bot
Copy link
Collaborator

All green in build 7 (ad5197b6dfaf955430f6af1971c2a769d9384325):

  • wasmtime/0.29.0@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 0bc6ff9 into conan-io:master Sep 9, 2021
@madebr madebr deleted the wasmtime_fix branch September 9, 2021 22:46
@redradist
Copy link
Contributor

redradist commented Sep 13, 2021

@madebr Seems like your change from source to binary broke number of configurations https://conan.io/center/wasmtime
For first revision there are much bigger number of different configurations !!

In your revision it is only 6 configurations in your revision https://conan.io/center/wasmtime?version=0.29.0&revision=b298dde16782c9f78164a16db7485e6a vs 32 in my original revision https://conan.io/center/wasmtime?version=0.29.0&revision=8f85e593d7dd82d674b3d722b790ca84 !!

@madebr
Copy link
Contributor Author

madebr commented Sep 13, 2021

It didn't break them.
What is done here is that all gcc@Linux + clang@Linux combinations are mapped to one single package.
Remember, wasmtime releases only one binary that can be used on Linux.
So it would be a waste of storage to serve multiple copies.
e.g. a package for gcc5@Linux would be the same as a package for clang12@Linux
The same applies for other arch/os/compiler combinations.

I hope this explains everything?

@redradist
Copy link
Contributor

@madebr Yeah, you are right ...
One strange thing I see there is not arm option at least for Linux

@madebr
Copy link
Contributor Author

madebr commented Sep 13, 2021

The package is not built/served by conan-center, but can be built locally.
Only x86_64 for Linux is built+tested+hosted at the moment: https://github.com/conan-io/conan-center-index/blob/master/docs/supported_platforms_and_configurations.md#linux

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.

6 participants