Conversation
| url: https://github.com/wasmi-labs/wasmi/archive/refs/tags/v0.42.1.tar.gz | ||
| patches: | ||
| "0.42.1": | ||
| - patch_file: "patches/0001-xrplf-0.42.1.patch" |
There was a problem hiding this comment.
It would be much more clear to have 3 patches (as I explained above).
And instead of xrplf in the name, let's add something more meaningful, like:
cmakeversionsfuel
There was a problem hiding this comment.
This patch is inseparable set of changes required to integrate into rippled. You can't create separate patches as the rippled project will be in broken state. This patch created by several people and is a sync point for them. The only thing that can be separated is version, but it is really only refactoring change which is not necessary to put it into individual patch.
This is not "changes to the wasmi" patches - it is "be able to integrate" patch. If there will be individual changes - they can be placed into separated patches.
There was a problem hiding this comment.
I'm not telling them not to apply them, but they are changing different things, and represent different features/problems which are needed to make it work for xrplf, so they should be kept as separate patches
There was a problem hiding this comment.
This patch fix only 1 problem - integration to the rippled. That its only purpose. As you can see there are no changes, or fixes to the original code.
|
@bthomee export workflow is not run here. Could you please make it work on PRs? |
That's intentional. We don't want the Conan registry to be polluted when someone is trying out new stuff; it should only be updated once it has been reviewed. Unfortunately, GitHub sucks. GitLab would allow you to add a manual step that only maintainers can click (or at least, which needs their approval), which would support running it as part of a PR. GitHub doesn't allow this. |
Sure. |
oleks-rip
force-pushed
the
wasmi
branch
2 times, most recently
from
5f2ba66 to
73a51f5
Compare
|
@mathbunnyru @oleks-rip It's perfectly fine, and I'd argue that it's recommended, to have a single patch file that spans multiple source files when these changes form part of a single unit of changes that, if split into separate patch files, wouldn't result in code that compiles when applied individually and separately. If they can be split, however, because they target different and independent functionality, I would suggest to do so. Regarding the |
|
I think I fixed all conan comments, no? |
Kind of yes, and kind of no.
they do target different and independent functionality, but it doesn't mean the code can be compiled when applied separately, because there are many separate things which need to be fixed To not argue more and waste too much time, I will let @bthomee decide, if patches should be split or not. Please, be explicit and tell your decision about patches name and what they include.
❤️ |
recipes/wasmi/all/conanfile.py
Outdated
| self.cpp_info.names["cmake_find_package"] = "wasmi" | ||
| self.cpp_info.names["cmake_find_package_multi"] = "wasmi" |
There was a problem hiding this comment.
| self.cpp_info.names["cmake_find_package"] = "wasmi" | |
| self.cpp_info.names["cmake_find_package_multi"] = "wasmi" | |
| self.cpp_info.set_property("cmake_file_name", "wasmi") | |
| self.cpp_info.set_property("cmake_target_name", "wasmi::wasmi") |
There was a problem hiding this comment.
I'm not sure this is 100% correct change, but using self.cpp_info.names gives warnings on usage
There was a problem hiding this comment.
I'm not sure not specifying anything is correct neither, could you please check what we should do?
There was a problem hiding this comment.
It do what we want without errors / warnings, so ye these lines were unnecessary.
mathbunnyru
left a comment
mathbunnyru
left a comment
There was a problem hiding this comment.
Also, I checked and wasmi depends on some system libraries, it should be reflected in package_info
https://github.com/wasmi-labs/wasmi/blob/main/crates/c_api/CMakeLists.txt#L83
There is no good reason to add them. |
| options = {"shared": [False]} | ||
| default_options = {"shared": False} |
There was a problem hiding this comment.
Most recipes I've seen have options for fPIC too (and then remove it for Windows). Why not add it here, and why not just do the same as all these other recipes?
options = {
"shared": [True, False],
"fPIC": [True, False],
}
default_options = {
"shared": False,
"fPIC": True,
}
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
There was a problem hiding this comment.
fpic is only for exe/shared libs
Remove options Remove verbose
Update wasmi patch
|
@mathbunnyru I'll go ahead and merge this. If there are any follow-ups needed we can do so next week. |
bthomee
dismissed
mathbunnyru’s stale review
Author responded to the requested changes comment. We can revisit this in a separate PR if needed.
3 participants
No description provided.