Skip to content

[ace] new optional feature: xml-utils#21334

Merged
BillyONeal merged 2 commits intomicrosoft:masterfrom
mitza-oci:ace-xml-utils
Nov 26, 2021
Merged

[ace] new optional feature: xml-utils#21334
BillyONeal merged 2 commits intomicrosoft:masterfrom
mitza-oci:ace-xml-utils

Conversation

@mitza-oci
Copy link
Copy Markdown
Contributor

@mitza-oci mitza-oci commented Nov 11, 2021

Added a feature to the ace port.

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

No change

Yes

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

Yes

Copy link
Copy Markdown

@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!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 86b23850c376428633e8690b6983028da0c93683 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/a-/ace.json b/versions/a-/ace.json
index 4e05683..f1b1f9d 100644
--- a/versions/a-/ace.json
+++ b/versions/a-/ace.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "9de0b3885f86f873aebe25cbbe5b4076d0110666",
+      "version": "7.0.3",
+      "port-version": 3
+    },
     {
       "git-tree": "67c57059b237310a1907da692fc75acb3d8d2726",
       "version": "7.0.3",
diff --git a/versions/baseline.json b/versions/baseline.json
index 1ff8371..93bdbd1 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -18,7 +18,7 @@
     },
     "ace": {
       "baseline": "7.0.3",
-      "port-version": 2
+      "port-version": 3
     },
     "activemq-cpp": {
       "baseline": "3.9.5",

@mitza-oci mitza-oci marked this pull request as ready for review November 11, 2021 17:38
Copy link
Copy Markdown

@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!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 7d3aeba6849b0be8517686e84c70f35c9f19f908 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/a-/ace.json b/versions/a-/ace.json
index acf05ee..f6c2215 100644
--- a/versions/a-/ace.json
+++ b/versions/a-/ace.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "2e49f11e749e936dd0f24230a6ed56e55efb4c8a",
+      "git-tree": "10d2e6d0027a3b5e1994b3235aab7963b7d2596a",
       "version": "7.0.3",
       "port-version": 4
     },

@JonLiu1993 JonLiu1993 self-assigned this Nov 12, 2021
@JonLiu1993 JonLiu1993 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Nov 12, 2021
@JonLiu1993
Copy link
Copy Markdown
Contributor

@mitza-oci, thanks for your pr, I tested all the features in the x86-windows triplt locally, but failed. This is the error log. Please take a look:
build-x86-windows-rel-out.log

@jwillemsen
Copy link
Copy Markdown
Contributor

@mitza-oci See #21322 for the upgrade to ACE 7.0.4

@mitza-oci
Copy link
Copy Markdown
Contributor Author

@mitza-oci See #21322 for the upgrade to ACE 7.0.4

Right, I'll rebase this again and check the error that @JonLiu1993 posted

@mitza-oci
Copy link
Copy Markdown
Contributor Author

Xerces-C++ has #define XERCES_XMLCH_T char16_t which is not what ACE expects with uses_wchar=1. I'm not sure if that's unique to the vcpkg port of Xerces or if it's an upstream problem

@mitza-oci
Copy link
Copy Markdown
Contributor Author

@jwillemsen in #5602 you added the optional xmlch-wchar feature to the xerces-c port, I guess we need to use it here

@mitza-oci mitza-oci marked this pull request as draft November 12, 2021 17:40
@jwillemsen
Copy link
Copy Markdown
Contributor

I think the existing ace xml feature was intended for xml_utils but looks it is now used for acexml. Looks we used this in the past for the azure pipelines

Copy link
Copy Markdown

@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 ace but no changes to version or port version.
-- Version: 7.0.3#3
-- Old SHA: c58c295074b4a00a80b4e684f9a876ed369cdd49
-- New SHA: 2e49f11e749e936dd0f24230a6ed56e55efb4c8a
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

Copy link
Copy Markdown

@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 ace but no changes to version or port version.
-- Version: 7.0.3#3
-- Old SHA: c58c295074b4a00a80b4e684f9a876ed369cdd49
-- New SHA: ca7679cf7da20230f44a4947ba756e23b8244214
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@mitza-oci
Copy link
Copy Markdown
Contributor Author

OK, @JonLiu1993 this should fix the error you saw. I didn't update version numbers since #21322 will get merged first. Once that's done I'll rebase this and update the versions.

@JonLiu1993
Copy link
Copy Markdown
Contributor

@mitza-oci ,Thanks for your pr, please run command "./vcpkg x-add-version ace --overwrite-version " then commit again

Copy link
Copy Markdown

@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 ace but no changes to version or port version.
-- Version: 7.0.3#3
-- Old SHA: c58c295074b4a00a80b4e684f9a876ed369cdd49
-- New SHA: ca7679cf7da20230f44a4947ba756e23b8244214
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 897ff9372f15c032f1e6cd1b97a59b57d66ee5b2 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/a-/ace.json b/versions/a-/ace.json
index e089726..f6081bc 100644
--- a/versions/a-/ace.json
+++ b/versions/a-/ace.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "ca7679cf7da20230f44a4947ba756e23b8244214",
+      "git-tree": "c58c295074b4a00a80b4e684f9a876ed369cdd49",
       "version": "7.0.3",
       "port-version": 3
     },

@mitza-oci mitza-oci marked this pull request as ready for review November 19, 2021 05:20
@mitza-oci mitza-oci requested a review from JonLiu1993 November 19, 2021 05:20
@mitza-oci
Copy link
Copy Markdown
Contributor Author

Do I need to do anything for requires:all-feature-testing?

@JonLiu1993
Copy link
Copy Markdown
Contributor

JonLiu1993 commented Nov 22, 2021

All features are tested successfully in the

Do I need to do anything for requires:all-feature-testing?

No, I am testing all features on my local machine

@JonLiu1993
Copy link
Copy Markdown
Contributor

@mitza-oci ,I tested all features on my local machine, x86-windows and x64-windows-static triplet are successfully, but x64-windows triplet have a little problem:

Mismatching number of debug and release binaries. Found 140 for debug but 134 for release.
Debug binaries

E:/test5/vcpkg/packages/ace_x64-windows/debug/lib/ACEsd.lib
E:/test5/vcpkg/packages/ace_x64-windows/debug/lib/ACEXMLsd.lib
......
Release binaries

E:/test5/vcpkg/packages/ace_x64-windows/lib/ACE.lib
E:/test5/vcpkg/packages/ace_x64-windows/lib/ACEXML.lib
......

Debug binaries were not found
Found 2 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile:
E:\test5\vcpkg\ports\ace\portfile.cmake
-- Performing post-build validation done
Stored binary cache: C:\Users\AppData\Local\vcpkg\archives\8c\8c6174664096c9668cbb96136932eb6bf2d06b34fa4009c2c8be0fc3b7b05951.zip
Installing package ace[core,ssl,tao,wchar,xml,xml-utils,zlib]:x64-windows...
Elapsed time for package ace:x64-windows: 3.413 h

@mitza-oci
Copy link
Copy Markdown
Contributor Author

If I'm reading that correctly, it generated ACEsd.lib even though it was not configured for static libs?
Nothing in this PR should have impacted that. I can try reproducing locally.

@mitza-oci
Copy link
Copy Markdown
Contributor Author

I didn't get an error running vcpkg install --recurse ace[core,ssl,tao,wchar,xml,xml-utils,zlib]:x64-windows on this branch.

@JonLiu1993
Copy link
Copy Markdown
Contributor

If I'm reading that correctly, it generated ACEsd.lib even though it was not configured for static libs? Nothing in this PR should have impacted that. I can try reproducing locally.

It’s a bit strange, I just clone your branch and test it locally

@JonLiu1993
Copy link
Copy Markdown
Contributor

All features are tested successfully in the following triplet:

  • x86-windows
  • x64-windows-static

@mitza-oci have tested x64-windows triplet successfully locally

@JonLiu1993 JonLiu1993 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Nov 24, 2021
@BillyONeal BillyONeal merged commit 679367e into microsoft:master Nov 26, 2021
@BillyONeal
Copy link
Copy Markdown
Member

Thanks for the new feature!

@mitza-oci mitza-oci deleted the ace-xml-utils branch November 29, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants