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

Improve wasmtime recipe #1

Closed
wants to merge 2 commits into from

Conversation

uilianries
Copy link

Hello!

Thank you for providing the initial PR for wasmtime. Some important points should be considered to include to CCI before. This PR provides all changes required to be approved:

  • Prebuilt artifacts are downloaded directly to build folder, not source
  • Homepage follows the project homepage, not github page as main url.
  • fPIC should not be used here because is not manageable. It will produce a warning, not an error.
  • Use Compiler minor version only, patch is not needed. It follows same pattern from settings.yml
  • Copy the artifacts according the configuration. Shared libraries should not be in a static package
  • Compiler should not be part of the package id, as it's a prebuilt package
  • The extension .dll.lib is not a MS pattern, and is not the first package which tries to use it in CCI.
  • Use TARGETS to avoid cmake definitions propagation, it can mask your test package result
  • Prefer cmake_find_package_multi to generate CONFIG. It generates more files == better test.

/cc @prince-chrismc


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

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Copy link

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@madebr
Copy link

madebr commented Aug 9, 2021

@uilianries
I failed to see your pr before I created #2. Sorry 😕

@uilianries
Copy link
Author

@madebr No problem at all! Thank you for continuing this!

redradist added a commit that referenced this pull request Sep 6, 2021
* Initial support for wasmtime package

* Fixed build package on Linux

* Updates according the comments in review #0

* Update recipes/wasmtime/all/conanfile.py

Update license type

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

* Updates according the comments in review #1

* Updates according the comments in review #2

* Updates according the comments in review conan-io#3

* Updates according the comments in review conan-io#4

* Updates according the comments in review conan-io#5

* Next iteration of fixes

* Fixed cmake variable C_STANDARD -> CMAKE_C_STANDARD

* Added check on minimal version of conan

* Used copytree instead of copy individual files

* Fixed the build

* Added checking for architechure in validate(...)

* Updated receipt for new comments from @madebr

* Fixed calling method tools.cross_building(...)

* Update def package(self) according the comments in review

* Updated versions of wasmtime

* Updated forgot version ugrade

* Fixed sha256sum for macos

* Fixed SHA256 for Linux

* Updated all SHA256 to proer values

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@redradist
Copy link
Owner

Close as it was merged already

@redradist redradist closed this Sep 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