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

[asmjit] Update to 2024-06-05 #39090

Closed
wants to merge 4 commits into from

Conversation

kobalicek
Copy link
Contributor

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@kobalicek
Copy link
Contributor Author

kobalicek commented Jun 2, 2024

Is it possible to at least see what failed in packages that depend on asmjit?

It seems it's all about a polyhook package, but I have no idea what is wrong, build logs are not available.

I removed the ARM restriction in the vcpkg.json file because asmjit builds well on ARM, at least according to AsmJits CI pipeline:

@kobalicek
Copy link
Contributor Author

kobalicek commented Jun 2, 2024

BTW I think that maybe it's related to the following lines removed:

if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
    vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/asmjit/core/api-config.h"
        "#if !defined(ASMJIT_STATIC)"
        "#if 0"
    )
endif()

But this change does the right thing!

If you consume asmjit from cmake, then cmake is responsible for setting ASMJIT_STATIC and I verified that it's properly exported by cmake. The definition is documented properly in asmjit's documentation too, so if anyone consumes AsmJit as a static library and doesn't use cmake then it's the responsibility of such project to define the flag explicitly as it's part of the contract.

I think vcpkg should not patch asmjit headers - there is no reason to do that; and I suggest the upstream should fix the tools that depend on asmjit instead, as that's the cleanest solution.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 2, 2024

x64-windows error (i.e. unrelated to header fixup):

D:\b\polyhook2\src\6141013c7f-4f5bdd1209.clean\sources\ILCallback.cpp(258): error C2039: 'init': is not a member of 'asmjit::_abi_1_13::FuncSignature'

x64-linux error, similar:

/mnt/vcpkg-ci/b/polyhook2/src/6141013c7f-4f5bdd1209.clean/sources/ILCallback.cpp: In member function ‘uint64_t PLH::ILCallback::getJitFunc(const string&, const std::vector<std::__cxx11::basic_string<char> >&, asmjit::_abi_1_13::Arch, PLH::ILCallback::tUserCallback, std::string)’:
/mnt/vcpkg-ci/b/polyhook2/src/6141013c7f-4f5bdd1209.clean/sources/ILCallback.cpp:258:13: error: ‘struct asmjit::_abi_1_13::FuncSignature’ has no member named ‘init’
  258 |         sig.init(getCallConv(callConv),asmjit::FuncSignature::kNoVarArgs, getTypeId(retType), args.data(), (uint32_t)args.size());
      |             ^~~~

cmake is responsible for setting ASMJIT_STATIC and I verified that it's properly exported by cmake.

Microsoft cares for VS users which use pure msbuild, not CMake.

@kobalicek
Copy link
Contributor Author

Regarding msbuild:

I'm not sure I can comment about msbuild as there is no msbuild support in AsmJit - if vcpkg only cares of VS msbuild users then I guess it should be clearly stated on its homepage? I've got the impression that vcpkg is a cross-platform solution that picks no sides. Since AsmJit uses cmake and this port depends on it I don't consider there is a problem here.

Regarding broken packages:

Is there any document/statement about not updating a package if it means that some other package in the whole vcpkg tree that depends on it cannot be built with that latest version? I cannot possibly fix all dependents - I only care of AsmJit and breaking changes are clearly documented here:

I think broken dependents should pin AsmJit at a specific version/commit instead of preventing AsmJit port update. Or how should this be even solved? I cannot update AsmJit without breaking other ports, but other ports cannot be updated to a newer AsmJit if AsmJit port is never updated... Or should a broken dependent deny lifetime updates of AsmJit in vcpkg?

As an author of AsmJit I just feel that it's irresponsible against AsmJit users to have outdated port on vcpkg that is known to have bugs at this point; and I would like to offer them a better support. However, I cannot possibly update all dependents that get broken with AsmJit port update as it's already used by a lot of existing software.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 2, 2024

if vcpkg only cares of VS msbuild users then I guess it should be clearly stated on its homepage?

This was not the point I wanted to make. I just pointed out that not everybody is using CMake.
(I'm a community member. I do not even use Visual Studio.)

As an author of AsmJit I just feel that it's irresponsible against AsmJit users to have outdated port on vcpkg that is known to have bugs at this point; and I would like to offer them a better support. However, I cannot possibly update all dependents that get broken with AsmJit port update as it's already used by a lot of existing software.

I think there will be maintainers to help out after the weekend. YMMV.

@jimwang118 jimwang118 self-assigned this Jun 3, 2024
@jimwang118 jimwang118 added the category:port-update The issue is with a library, which is requesting update new revision label Jun 3, 2024
@stevemk14ebr
Copy link
Contributor

Is there any document/statement about not updating a package if it means that some other package in the whole vcpkg tree that depends on it cannot be built with that latest version?

This is unfortunately a tough situation with vcpkg. From what I understand (I may be wrong) vcpkg needs packages to always be up to to date. I fought a very similar battle even getting polyhook, asmjit, and asmtk into vcpkg in the first place. There was a complex dependency tree and I had to submit PRs here in vcpkg and to each of those package repos then ensure they all landed at the same time to get everything into vcpkg.

I will fix the outdated API polyhook relies on so this can be resolved. Please give me a few days

@stevemk14ebr
Copy link
Contributor

I submitted a package update which will fill polyhook once asmjit port is updated here.

@kobalicek
Copy link
Contributor Author

Thanks a lot, that was fast!

Maybe this should be reiterated then? Anything else to do to get this done?

@stevemk14ebr
Copy link
Contributor

I believe we need a vcpkg maintainer to confirm everything is ok now and complete the merge of both PRs at the same time.

@kobalicek
Copy link
Contributor Author

I still think that having a build tree of 2428 packages developed totally independently to always build is a heroic effort. Of course we should not break other's code, but sometimes it's inevitable to fix bugs or to add new features.

@jimwang118
Copy link
Contributor

@kobalicek @stevemk14ebr Thank you for your update and reply. However, asmjit is called as the upstream of polyhook2, and the FuncSignature::init function is deleted in the new asmjit, which causes polyhook2 to fail to compile on CI. Therefore, this error needs to be fixed in this PR before CI can pass.

@stevemk14ebr
Copy link
Contributor

Are you suggesting to do the changes from #39105 within this PR?

@dg0yt
Copy link
Contributor

dg0yt commented Jun 4, 2024

Are you suggesting to do the changes from #39105 within this PR?

If they must be updated together, then it must be done in one PR.

Maybe this should be reiterated then? Anything else to do to get this done?

On arm64-osx, fbgemm needs to be looked at.

@kobalicek
Copy link
Contributor Author

fbgemm upstream is already using a recent AsmJit, but vcpkg is not using fbgemm upstream...

There is still this bug regarding ARM open:

pytorch/FBGEMM#2074

So I'm not sure fbgemm is even supposed to build on ARM.

This is honestly beyond my time limit so I'm giving up on this.

@stevemk14ebr
Copy link
Contributor

stevemk14ebr commented Jun 4, 2024

If they must be updated together, then it must be done in one PR.

The requirement for them to all be together is coming from vcpkg. These are independent projects.

Polyhook dependencies:

  • asmtk
  • asmjit
  • zydis

FBGEMM dependencies:

  • asmjit
  • ... others ...

So when updating asmjit the vcpkg maintainers are saying we need to also update the upstream sources. That doesn't seem right to me, we should let these upstream packages temporarily break until their respective maintainers do the work to fix them. OR vcpkg adds proper versionioning to avoid the temporary breakage, letting the upstream sources do the migration when they have time.

I have already done the work for Polyhook update in #39105.

@kobalicek kobalicek changed the title [asmjit] Update to 2024-05-31 [asmjit] Update to 2024-06-05 Jun 5, 2024
@kobalicek
Copy link
Contributor Author

I'm wondering what to do about fbgemm.

I tried on ARM64 Apple to build it in the current tree and it refuses to build it as AsmJit previously had "!arm" in the package supported list, so it transitioned to fbgemm.

This means that removing this restriction now allows fbgemm to be build on arm, so I think I should just add "!arm" to fbgemm instead so it's not built on this platform, agreed?

I don't feel like fixing something that never worked in the first place.

@kobalicek
Copy link
Contributor Author

@stevemk14ebr I have already included your changes in this PR, you can close your PR, thanks a lot for a quick fix!

@jimwang118
Copy link
Contributor

Usage test pass with following triplets:

x64-windows
x64-windows-static

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Jun 7, 2024
@kobalicek
Copy link
Contributor Author

Amazing, hopefully this could be merged soon as I need to update one more package, which depends on the latest asmjit, thanks!

Comment on lines -24 to -29
if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/asmjit/core/api-config.h"
"#if !defined(ASMJIT_STATIC)"
"#if 0"
)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

As @dg0yt pointed out, these lines are necessary for the library to work properly for MSBuild users and shouldn't affect the package adversely.

Copy link
Contributor Author

@kobalicek kobalicek Jun 7, 2024

Choose a reason for hiding this comment

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

Please can you elaborate?

Are msbuild users not able to setup compile-time macros? If anyone wants statically linked AsmJit there is ASMJIT_STATIC macro that does exactly that and this is a documented behavior (basically a contract).

So, are you suggesting that ALL vcpkg packages patch source code to define/undefine a visibility macro? Because I don't see that in other packages, so I don't think AsmJit should be an exception here.

What I'm trying to do is to have vanilla AsmJit on vcpkg - not patched, 100% upstream. The cmake build system works correctly and dependents link properly. So far I haven't seen any non-cmake requirement in vcpkg documentation. There are no tests in vcpkg registry that would explicitly test msbuild without cmake integration. So, I'm really confused about this.

Moreover, if there is any extra requirement that ALL ports must follow, please link me the relevant documentation so I can at least read about those requirements - I haven't found any, the whole vcpkg build system is based on cmake, and AsmJit's cmake integration works as advertised.

AsmJit is not a difficult project to compile - it doesn't even need a build system, and the ASMJIT_STATIC macro is the only thing that must be defined in order to consume it statically. I think that if you have a custom msbuild project, that consumes dependencies from vcpkg but totally ignores the configuration, this is your burden, and not vcpkg port burden (or in other words - my burden).

Copy link
Contributor

Choose a reason for hiding this comment

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

Because I don't see that in other packages, so I don't think AsmJit should be an exception here.

look again. (aws-sdk-cpp; cjson; freerdp; graphviz; hunspell; libheif, etc)

What I'm trying to do is to have vanilla AsmJit here - not patched, 100% upstream

Not possible if there are visibility macros

I know that this patching is not right

Incorrect. The patching is correct since vcpkg always only installs one type (static/shared) of a library. Downstream consumers shouldn't need to know if something is build static or shared. However, the macro needs additional knowledge of that otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please link me the documentation that describes this behavior first. I want to read about this flaw.

Copy link
Contributor

Choose a reason for hiding this comment

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

the whole vcpkg build system is based on cmake,

Vcpkg uses cmake as a scripting language, and it provides some integration.

But there are many ports which do not provide CMake packages. Some provide only pkg-config. Some provide nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I just don't like this situation.

They want from me to do something that is not documented nor stated anywhere and I dislike it very much from a maintainer's perspective. And all of this is about some phantom "MSBuild users" that need flawless integration with vcpkg that means patching source code of other projects... But where are they? Can at least one MSBuild user explain to me why adding ASMJIT_STATIC to MSBuild file and thus following the AsmJit contract is such a big deal?

I feel this is just very hypothetical and I'm being nitpicked to support a scenario that doesn't exist in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kobalicek: #20936 found via git blame.
asmjit installs wrong headers since it only allows building either static or shared as such the installed headers should reflect that. There is no reason to have a magic define for downstream users which is not necessary since it is decided at build time which library will be installed and thus used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by installing wrong headers? The headers are the same in both configurations!

I think that the issue above clearly describes the problem. Instead of helping the user to properly setup his project to consume the library, the port was patched instead. However, this essentially did create a problem which I'm now trying to undo.

Copy link
Contributor

Choose a reason for hiding this comment

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

The headers are the same in both configurations!

No they are not. You are just hiding that fact with a preprocessor define. If they would be the same you wouldn't need the preprocessor to change them conditionally. (which on windows even changes the symbol names!). This is conceptually the same as having configure_file in cmake and directly install the header with the correct defines set or have the cmake build use https://cmake.org/cmake/help/latest/module/GenerateExportHeader.html.

However, this essentially did create a problem which I'm now trying to undo.

There is nothing to undo. You just don't like the solution.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, MSBuild is an important customer that doesn't understand CMake configs but it is far from the only customer that doesn't understand them. Plain make or similar downstream customers prefer not needing to replicate settings that are known at library build time too.

versions/f-/fbgemm.json Outdated Show resolved Hide resolved
@stevemk14ebr
Copy link
Contributor

This is a sad conclusion. I put in work to quickly fix polyhook. Please reopen this and just keep the patch in so we can all get along.

@kobalicek
Copy link
Contributor Author

kobalicek commented Jun 15, 2024

@stevemk14ebr

I have decided I'm not going to contribute to vcpkg anymore as there is no clear documentation about the issue that was discussed above, which is surprisingly a blocker.

AsmJit clearly specifies how to build and use it, and it offers feature selection and static/shared build selection via a compile-time macro, which is clearly documented on AsmJit's homepage. Cmake users have the advantage of using asmjit::asmjit target that already takes care of that. Msbuild users, however, should follow the official documentation and not rely on a package manager to patch headers for them. Any project that uses asmjit and relies on a package manager to patch headers for it I consider broken.

After exploring more pull requests here on vcpkg, I have concluded that this unclear requirement costs me and many others a lot of time and it seems it's only for users that cannot even setup their msbuild projects properly. I insist on having vanilla AsmJit here on vcpkg and I think it's a step forward. If vcpkg maintainers think differently I really suggest to state that in the documentation and to describe how this integration with msbuild should work, and more importantly, I think that the CI pipeline should test that too, because this PR was green, but still rejected.

Because at the moment I have the following impression: if consuming X is a set of compiler and linker flags, you cannot just remove them both and say consuming X is just linking to libX and that's it. This would only work for minority of libraries, and especially it would not work for libraries that have dependencies. If I added a dependency to AsmJit it would break all of those fragile msbuild projects that @Neumann-A and @BillyONeal talked about...


I initially wanted to maintain this port, to submit new versions when an important bug-fix or feature gets merged, but I have reconsidered this as I don't want to maintain ports that patch AsmJit headers for literally no reason. If anyone wants an updated version, make your own PR. I haven't deleted my PR, so if anyone is interested, just fork this PR, add what vcpkg maintainers want, and make another PR.

@JakubMelka
Copy link
Contributor

I will be very happy, if a new version of Blend2D will be on VCPKG. There are important features. If I understand it correctly, there is a problem with this:

if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
    vcpkg_replace_string("${CURRENT_PACKAGES_DIR}/include/asmjit/core/api-config.h"
        "#if !defined(ASMJIT_STATIC)"
        "#if 0"
    )
endif()

Why VCPKG dont just define ASMJIT_STATIC?

@kobalicek
Copy link
Contributor Author

@JakubMelka It actually does define it, and since there is windows-static runner it's proven it works.

The whole issue here is about patching the source code to make projects that are lazy to define it working.

@JakubMelka
Copy link
Contributor

@kobalicek, Is there a list of these projects? Maybe I can help with that. I strongly need new version of Blend2D in VCPKG and I can spare some time helping to fix this issue.

@kobalicek
Copy link
Contributor Author

@JakubMelka If you want to spend time on this just fork this PR (my branch) and add the code patching that I have removed back. It's just for some invisible MSBuild users and vcpkg maintainers who rely on code patching.

However, I would update the version as AsmJit in this PR is already behind (missing a bug fix).

Once this is done I can help with Blend2D as it has some new cmake options to avoid patching.........

@JakubMelka
Copy link
Contributor

@kobalicek, @stevemk14ebr, I have prepared a new pull request, where I have reverted removal of the code. #39630

JavierMatosD pushed a commit that referenced this pull request Jul 5, 2024
Co-authored-by: Petr Kobalicek <[email protected]>
Co-authored-by: Stephen Eckels <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants