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

[zeroc-ice] New port #23764

Merged
merged 35 commits into from
Apr 13, 2022
Merged

[zeroc-ice] New port #23764

merged 35 commits into from
Apr 13, 2022

Conversation

bold84
Copy link
Contributor

@bold84 bold84 commented Mar 25, 2022

Describe the pull request

  • What does your PR fix?

    Fixes ZeroC Ice Support #6106

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    windows, yes (still working on linux / mac)

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for zeroc-ice but no changes to version or port version.
-- Version: 3.7.7
-- Old SHA: 38305b2ef3fe0742978b9b6bc101afb1f6141161
-- New SHA: 99a4c4aefadf9f4110fc0e7ce769d71eaf48d7f8
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@bold84 bold84 changed the title [zeroc-ice] Initial commit [zeroc-ice] New port Mar 25, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for zeroc-ice but no changes to version or port version.
-- Version: 3.7.7
-- Old SHA: b0871b1fa5b10cdb71df1b110de4d7ca0dc0cc57
-- New SHA: 4cecb516e83aba9fb9c56153b7fa1f7040143aae
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for zeroc-ice but no changes to version or port version.
-- Version: 3.7.7
-- Old SHA: b0871b1fa5b10cdb71df1b110de4d7ca0dc0cc57
-- New SHA: 4cecb516e83aba9fb9c56153b7fa1f7040143aae
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Mar 25, 2022
@bold84
Copy link
Contributor Author

bold84 commented Mar 25, 2022

The build succeeds, but this is an issue I'm not sure how to resolve in a way that's accepted here.

Ice requires the usage of an IDL file compiler, which I download in the powershell script before the build starts.

vcpkg_install_msbuild() doesn't let me remove these files before "installing" the binaries to the package folder.
I could use vcpkg_execute_build_process() instead of vcpkg_install_msbuild() but I'm not sure if that's okay for you guys.

As you can see in the portfile, I actually delete the fils from the packages directory.

Screen Shot 2022-03-25 at 14 21 08

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for zeroc-ice but no changes to version or port version.
-- Version: 3.7.7
-- Old SHA: 4cecb516e83aba9fb9c56153b7fa1f7040143aae
-- New SHA: 37beab9f818399c78da3afe9f1bcf49ad706136a
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@LilyWangLL
Copy link
Contributor

Now the pipeline error is The folder /include is empty or not present. This indicates the library was not correctly installed. the header file install incorrect.

@LilyWangLL
Copy link
Contributor

zeroc-ice:x64-windows-static build failed on pipeline with the following error:

-- Installing: D:/packages/zeroc-ice_x64-windows-static/share/zeroc-ice/copyright
-- Performing post-build validation
Invalid crt linkage. Expected Debug,Static, but the following libs had:

    D:\packages\zeroc-ice_x64-windows-static\debug\lib\iceutild.lib: Debug,Dynamic
    D:\packages\zeroc-ice_x64-windows-static\debug\lib\sliced.lib: Debug,Dynamic

To inspect the lib files, use:
    dumpbin.exe /directives mylibfile.lib
Invalid crt linkage. Expected Release,Static, but the following libs had:

    D:\packages\zeroc-ice_x64-windows-static\lib\iceutil.lib: Release,Dynamic
    D:\packages\zeroc-ice_x64-windows-static\lib\slice.lib: Release,Dynamic

To inspect the lib files, use:
    dumpbin.exe /directives mylibfile.lib
Found 2 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile:
    C:\a\1\s\ports\zeroc-ice\portfile.cmake

@bold84 bold84 marked this pull request as ready for review March 25, 2022 10:47
@ghost
Copy link

ghost commented Mar 25, 2022

If I recall correctly, the CRT linkage was the reason why I opted to integrate CMake into Ice. There are also slice files that need to be available for the FindIce() feature to work correctly and I don't recall if the MSBuild recipe worked with that. I recall the cleanup in vcpkg will delete some binary files.

@bold84
Copy link
Contributor Author

bold84 commented Mar 25, 2022

If I recall correctly, the CRT linkage was the reason why I opted to integrate CMake into Ice. There are also slice files that need to be available for the FindIce() feature to work correctly and I don't recall if the MSBuild recipe worked with that. I recall the cleanup in vcpkg will delete some binary files.

I removed the hard coded crt linkage from the project files.

But good point, I need to add the slice files to the installation procedure. But I first have to get Linux and Mac working properly.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for zeroc-ice but no changes to version or port version.
-- Version: 3.7.7
-- Old SHA: 4f02050efcce648670e93d0b61888170d682c79b
-- New SHA: 0e4844d7863cb54220f9559c4388c7d023a603b7
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@bold84
Copy link
Contributor Author

bold84 commented Mar 31, 2022

Done.

I apologize for abusing the PR by pushing to this branch many times. I used this branch to transfer changes between different operating systems and architectures. In the future I would use a separate branch for this purpose. 😁

The port should build for Linux(x86,x64,arm,arm64), MacOS (x86,x64,arm64) and Windows (x86,x64), in shared and static build configurations. ZeroC's VS project files do not have configurations for arm and it would need a bit of work to make this work, because the slice compiler has to be built for the host machine as it's needed during the build of the subsequent components.

I will add Android, iOS and possibly Webassembly in separate PRs in the near future. The latter needs some work on Ice itself though.

@bold84
Copy link
Contributor Author

bold84 commented Mar 31, 2022

Why does it take so long for the commits to be built? Is there some kind of throttling?

@dg0yt
Copy link
Contributor

dg0yt commented Mar 31, 2022

Why does it take so long for the commits to be built? Is there some kind of throttling?

No, it is congestion.

  • Recent baseline regressions caused a backlog.
  • A broken tool/CI interaction causes PR CI runs to build ports which are known to fail.
  • The recent zlib update invalidated a lot of cached artifacts.
  • Some of the queued PRs need a lot of build time.
  • At least one port with significant build time fails to upload to cache.

Take a look at the queue:
https://dev.azure.com/vcpkg/public/_build?definitionId=27&_a=summary

@bold84
Copy link
Contributor Author

bold84 commented Mar 31, 2022

Why does it take so long for the commits to be built? Is there some kind of throttling?

No, it is congestion.

  • Recent baseline regressions caused a backlog.
  • A broken tool/CI interaction causes PR CI runs to build ports which are known to fail.
  • The recent zlib update invalidated a lot of cached artifacts.
  • Some of the queued PRs need a lot of build time.
  • At least one port with significant build time fails to upload to cache.

Take a look at the queue: https://dev.azure.com/vcpkg/public/_build?definitionId=27&_a=summary

Thank you for the explanation. 😊

@LilyWangLL
Copy link
Contributor

@strega-nil-ms Could you please review this PR again?

@bold84
Copy link
Contributor Author

bold84 commented Apr 9, 2022

How many weeks does it typically take until you guys review change requests? ☺️

@LilyWangLL
Copy link
Contributor

Ping @strega-nil

@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Apr 12, 2022
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

merging once green!

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for zeroc-ice but no changes to version or port version.
-- Version: 3.7.7
-- Old SHA: 7b7a2836ffa07e08a43a7aa58b8de996d3902751
-- New SHA: bdb180069d461c04c1eee1a7937afa63d0ea752b
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@strega-nil-ms strega-nil-ms merged commit 1c9b23b into microsoft:master Apr 13, 2022
@ghost
Copy link

ghost commented Apr 13, 2022

Done.

I apologize for abusing the PR by pushing to this branch many times. I used this branch to transfer changes between different operating systems and architectures. In the future I would use a separate branch for this purpose. grin

The port should build for Linux(x86,x64,arm,arm64), MacOS (x86,x64,arm64) and Windows (x86,x64), in shared and static build configurations. ZeroC's VS project files do not have configurations for arm and it would need a bit of work to make this work, because the slice compiler has to be built for the host machine as it's needed during the build of the subsequent components.

I will add Android, iOS and possibly Webassembly in separate PRs in the near future. The latter needs some work on Ice itself though.

Perhaps these will help with icebuilder and arm:
mumble-voip/ice@a9d68a8
https://github.com/mumble-voip/ice/blob/3.7/icebuilder/CMakeLists.txt

@bold84
Copy link
Contributor Author

bold84 commented Apr 13, 2022

@ZeroAbility Thanks!

At the moment re-doing the skia port. I will look into this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZeroC Ice Support
7 participants