Skip to content

[chartdir] use versioned download link#23732

Merged
BillyONeal merged 10 commits intomicrosoft:masterfrom
cd-pkwan:master
Jun 14, 2022
Merged

[chartdir] use versioned download link#23732
BillyONeal merged 10 commits intomicrosoft:masterfrom
cd-pkwan:master

Conversation

@cd-pkwan
Copy link
Contributor

The original download link is a generic link that points to the latest version of the software. The download content can change from time to time, and this will break the SHA512 hash in the port. This update change the download link to a link that points to the real file with version number, which will not change. This makes the port more stable.

@strega-nil-ms strega-nil-ms changed the title Update download link [chartdir] use versioned download link Mar 23, 2022
@strega-nil-ms
Copy link
Contributor

@cd-pkwan I assume you're the author, since the website has been changed to mention vcpkg; would it be possible for you to do the same for the macOS and Linux versions?

@JonLiu1993 JonLiu1993 self-assigned this Mar 24, 2022
@JonLiu1993 JonLiu1993 added category:port-update The issue is with a library, which is requesting update new revision requires:author-response labels Mar 24, 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 chartdir but no changes to version or port version.
-- Version: 7.0.0#3
-- Old SHA: 2b4bad1eb46439cbd63295d8120f03966c52d162
-- New SHA: 7b689d64616cb407757ebc0e4427b39c77d72bd6
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@JonLiu1993 JonLiu1993 linked an issue Mar 24, 2022 that may be closed by this pull request
@ghost
Copy link

ghost commented Mar 24, 2022

CLA assistant check
All CLA requirements met.

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.

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/chartdir/vcpkg.json

Valid values for the license field can be found in the documentation

@JonLiu1993
Copy link
Contributor

@cd-pkwan ,Thanks for your pr, could you please sign in CLA first?

BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Mar 28, 2022
BillyONeal added a commit that referenced this pull request Mar 28, 2022
* Update vcpkg-tool to 2022-03-24

* Hook up microsoft/vcpkg-tool#345

* Hook up microsoft/vcpkg-tool#442

* Update vcpkg-tool to 2022-03-25

* Analysis of failures.

* [Most recent nightly build failed](https://dev.azure.com/vcpkg/public/_build/results?buildId=69427)
* [Validation of this tool update failed](https://dev.azure.com/vcpkg/public/_build/results?buildId=69417)

## Common to both:

PASSING, REMOVE FROM FAIL LIST: chartdir:x64-windows (.\scripts\ci.baseline.txt)
PASSING, REMOVE FROM FAIL LIST: chartdir:x64-windows-static-md (.\scripts\ci.baseline.txt)
PASSING, REMOVE FROM FAIL LIST: chartdir:x86-windows (.\scripts\ci.baseline.txt)

Probably fixed by #23701

PASSING, REMOVE FROM FAIL LIST: gmp:x64-uwp (.\scripts\ci.baseline.txt)
PASSING, REMOVE FROM FAIL LIST: gmp:x64-windows-static-md (.\scripts\ci.baseline.txt)

Probably fixed by #23466 ?

REGRESSION: colmap:x64-windows-static-md failed with BUILD_FAILED. If expected, add colmap:x64-windows-static-md=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.

I don't know exactly what changed. I observe that
* this thing depends on a *lot* of stuff
* on March 14 we didn't even attempt to build this
* the x64-windows ones are already in the baseline

so I skipped it.

REGRESSION: qtdeclarative:x64-windows. If expected, add qtdeclarative:x64-windows=fail to .\scripts\ci.baseline.txt.
REGRESSION: qtdeclarative:x64-windows failed with BUILD_FAILED. If expected, add qtdeclarative:x64-windows=fail to C:\a\1\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtdeclarative:x64-windows failed with BUILD_FAILED. If expected, add qtdeclarative:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtdeclarative:x64-windows failed with BUILD_FAILED. If expected, add qtdeclarative:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtdeclarative:x64-windows failed with BUILD_FAILED. If expected, add qtdeclarative:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtdeclarative:x64-windows failed with BUILD_FAILED. If expected, add qtdeclarative:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtdeclarative:x64-windows failed with BUILD_FAILED. If expected, add qtdeclarative:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: qtdeclarative:x64-windows failed with BUILD_FAILED. If expected, add qtdeclarative:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.

This is a reporting change: The new world order also includes host build failures which is why it's duplicated.

See also #23714
See also #23490

I'm nervous about baslining this because it seems most of the qt world is built on top of this port

I filed #23824 about this and @Neumann-A indicated this should be fixed by #23755

REGRESSION: nettle:x64-uwp. If expected, add nettle:x64-uwp=fail to .\scripts\ci.baseline.txt.
REGRESSION: nettle:x64-windows-static-md. If expected, add nettle:x64-windows-static-md=fail to .\scripts\ci.baseline.txt.
REGRESSION: nettle:x64-uwp failed with BUILD_FAILED. If expected, add nettle:x64-uwp=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.
REGRESSION: nettle:x64-windows-static-md failed with POST_BUILD_CHECKS_FAILED. If expected, add nettle:x64-windows-static-md=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.

Didn't analyze, probably fixed by #23519 ?

REGRESSION: libgpg-error:x64-uwp. If expected, add libgpg-error:x64-uwp=fail to .\scripts\ci.baseline.txt.
REGRESSION: libgpg-error:x64-uwp failed with BUILD_FAILED. If expected, add libgpg-error:x64-uwp=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.

This was broken by VS2022 update:
```
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VisualStudio\v17.0\AppxPackage\Microsoft.AppXPackage.Targets(892,25): error MSB4086: A numeric comparison was attempted on "$(TargetPlatformMinVersion)" that evaluates to "" instead of a number, in condition "'$(TargetPlatformMinVersion)' >= '10.0.17200.0'". [C:\Dev\vcpkg\buildtrees\libgpg-error\x64-uwp-rel\error-1.42-2324ddbc71.clean\SMP\libgpg-error_winrt.vcxproj]
```

REGRESSION: libmikmod:x64-osx. If expected, add libmikmod:x64-osx=fail to .\scripts\ci.baseline.txt.
REGRESSION: libmikmod:x64-osx failed with BUILD_FAILED. If expected, add libmikmod:x64-osx=fail to /Users/vagrant/Data/work/2/s/scripts/azure-pipelines/../ci.baseline.txt.

Broken between [2022-03-16](https://dev.azure.com/vcpkg/public/_build/results?buildId=68947) and [2022-03-18](https://dev.azure.com/vcpkg/public/_build/results?buildId=69051). Unfortunately I don't see obvious reasons why. Nothing else depends on this and nobody has noticed in 2 weeks, so I'm baslining it for now. (Will investigate shortly...)

## Only broken in tool update:

REGRESSION: mesa:x64-windows failed with BUILD_FAILED. If expected, add mesa:x64-windows=fail to C:\a\2\s\scripts\azure-pipelines/../ci.baseline.txt.

```
-- Downloading https://gitlab.freedesktop.org/mesa/mesa/-/archive/mesa-21.2.5/mesa-mesa-21.2.5.tar.gz -> mesa-mesa-mesa-21.2.5-1.tar.gz...
-- Extracting source /Users/vagrant/Data/downloads/mesa-mesa-mesa-21.2.5-1.tar.gz
-- Applying patch swravx512-post-static-link.patch
-- Applying patch swr-msvc-2.patch
-- Applying patch swr-llvm13.patch
-- Applying patch radv-msvc-llvm13-2.patch
-- Applying patch d3d10sw.patch
-- Using source at /Users/vagrant/Data/buildtrees/mesa/src/esa-21.2.5-2df234d2b1.clean
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'mako'
CMake Error at ports/mesa/portfile.cmake:85 (message):
  Python package 'mako' needs to be installed for port 'mesa'.

  Complete list of required python packages: setuptools;mako
Call Stack (most recent call first):
  ports/mesa/portfile.cmake:91 (vcpkg_get_python_package)
  scripts/ports.cmake:145 (include)
```

Looks like this is being tracked by #23089 ; perhaps that we don't have as aggressive a recycling strategy for macos boxes as we do for the others has let different machines give different results?

## Only broken without tool update:

REGRESSION: chromium-base:x64-osx. If expected, add chromium-base:x64-osx=fail to .\scripts\ci.baseline.txt.

This one has been constantly flaky; I baselined it.

REGRESSION: libxml2:x64-osx. If expected, add libxml2:x64-osx=fail to .\scripts\ci.baseline.txt.

This port uses vcpkg_from_git and the upstream server was down during the build.

* Restore chartdir to the baseline, I thought #23732 had been merged.
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.

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/chartdir/vcpkg.json

Valid values for the license field can be found in the documentation

@BillyONeal
Copy link
Member

I removed the skips from 671db8a from ci.baseline.txt , so if we get versioned links for MacOS and Linux we can finally start testing chardir regularly :)

Updated to use versioned download links for macOS, Linux x86 and Linux x64, in addition to Windows.
Copy link
Contributor Author

@cd-pkwan cd-pkwan left a comment

Choose a reason for hiding this comment

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

Updated to use versioned download link for macOS, Linux x86 and Linux x64.

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!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 38ea7762498f4040b51d17bef9218e755934f3cb -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/c-/chartdir.json b/versions/c-/chartdir.json
index a8aeb8a..6dc299d 100644
--- a/versions/c-/chartdir.json
+++ b/versions/c-/chartdir.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "470db0162f8e6a30ded91a22d8611e1a2fee2aac",
+      "git-tree": "1cc0eca76a11a651585beeefe4eae9abcd557fa0",
       "version": "7.0.0",
       "port-version": 4
     },

You have modified or added at least one vcpkg.json where a "license" field is missing.

Details

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/chartdir/vcpkg.json

Valid values for the license field can be found in the documentation

@BillyONeal
Copy link
Member

BillyONeal commented Mar 29, 2022

Building package chartdir[core]:x64-windows-static...
-- Note: chartdir only supports dynamic library linkage. Building dynamic library.
CMake Error at scripts/cmake/vcpkg_check_linkage.cmake:61 (message):
  Refusing to build unexpected dynamic library against the static CRT.

      If this is desired, please configure your triplet to directly request this configuration.
Call Stack (most recent call first):
  ports/chartdir/portfile.cmake:1 (vcpkg_check_linkage)
  scripts/ports.cmake:145 (include)

Should we add "supports": "!staticcrt" to vcpkg.json?

The other platforms are broken because:

if(TRIPLET_SYSTEM_ARCH MATCHES "arm" OR VCPKG_CMAKE_SYSTEM_NAME STREQUAL "WindowsStore" OR VCPKG_LIBRARY_LINKAGE STREQUAL static)

    set(VCPKG_POLICY_EMPTY_PACKAGE enabled)

is entered, so SOURCE_PATH does not get set, so

file(COPY "${SOURCE_PATH}/LICENSE.TXT" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}/copyright")

fails.

Proposal:

From 7d16e8044a677537c24564cce898ebdedeed3772 Mon Sep 17 00:00:00 2001
From: Billy Robert O'Neal III <bion@microsoft.com>
Date: Tue, 29 Mar 2022 10:52:41 -0700
Subject: [PATCH] Guard with supports rather than making the package empty.

---
 ports/chartdir/portfile.cmake | 6 +-----
 ports/chartdir/vcpkg.json     | 3 ++-
 versions/c-/chartdir.json     | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/ports/chartdir/portfile.cmake b/ports/chartdir/portfile.cmake
index bfeb10699..b263588f3 100644
--- a/ports/chartdir/portfile.cmake
+++ b/ports/chartdir/portfile.cmake
@@ -1,10 +1,6 @@
 vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)
 
-if(TRIPLET_SYSTEM_ARCH MATCHES "arm" OR VCPKG_CMAKE_SYSTEM_NAME STREQUAL "WindowsStore" OR VCPKG_LIBRARY_LINKAGE STREQUAL static)
-
-    set(VCPKG_POLICY_EMPTY_PACKAGE enabled)
-
-elseif(VCPKG_TARGET_IS_WINDOWS)
+if(VCPKG_TARGET_IS_WINDOWS)
 
     vcpkg_download_distfile(ARCHIVE_FILE
         URLS "https://www.advsofteng.com/vcpkg/chartdir_cpp_win_7.0.0.zip"
diff --git a/ports/chartdir/vcpkg.json b/ports/chartdir/vcpkg.json
index 425a2ca91..b6a1cfa0a 100644
--- a/ports/chartdir/vcpkg.json
+++ b/ports/chartdir/vcpkg.json
@@ -3,5 +3,6 @@
   "version": "7.0.0",
   "port-version": 4,
   "description": "ChartDirector is a powerful chart component for creating professional looking charts for web and windows applications.",
-  "homepage": "https://www.advsofteng.com/"
+  "homepage": "https://www.advsofteng.com/",
+  "supports": "!staticcrt & !arm & !uwp"
 }
diff --git a/versions/c-/chartdir.json b/versions/c-/chartdir.json
index a8aeb8aab..d7186fafa 100644
--- a/versions/c-/chartdir.json
+++ b/versions/c-/chartdir.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "470db0162f8e6a30ded91a22d8611e1a2fee2aac",
+      "git-tree": "6b1b1029aa38c2a698a4cd6e3cfa7104a3580baf",
       "version": "7.0.0",
       "port-version": 4
     },
-- 
2.35.1.windows.2

@cd-pkwan
Copy link
Contributor Author

My understanding of x64-windows-static is that the package should work with the static C runtime. In this case, the chartdir, though being a DLL, should work with the CRT static library. I often create projects in Visual Studio that use the /MT option to link with the static C runtime and it works normally. Does the x64-windows-static actually test if the package itself is a static library?

@BillyONeal
Copy link
Member

My understanding of x64-windows-static is that the package should work with the static C runtime.

We have blocks around attempting to use DLLs with the static CRT because it gives each DLL their own copy of the CRT. So the DLL interface can't contain std::string* or FILE* or otherwise interact with any CRT entity, which substantially all libraries we have seen are unprepared to deal with.

Does the x64-windows-static actually test if the package itself is a static library?

I don't know if we have an explicit post-build check for it but if a port does otherwise that port is extremely badly behaved. People who pick static want single-exe deployment.

@BillyONeal
Copy link
Member

We have blocks around attempting to use DLLs with the static CRT because it gives each DLL their own copy of the CRT. So the DLL interface can't contain std::string* or FILE* or otherwise interact with any CRT entity, which substantially all libraries we have seen are unprepared to deal with.

That said it looks like bchartdir.h indicates that this is intentionally done at the moment.

I think you can just do:

# ChartDirector's DLL interface only contains primitive types, so it is CRT agnostic.
if("${VCPKG_LIBRARY_LINKAGE}" STREQUAL "static")
    message(STATUS "Note: ${PORT} only supports dynamic library linkage. Building dynamic library.")
    set(VCPKG_LIBRARY_LINKAGE dynamic)
endif()

or similar in portfile.cmake rather than using vcpkg_check_linkage. vcpkg_check_linkage is the 99% case but you're kinda juggling razor blades :)

@JonLiu1993
Copy link
Contributor

@cd-pkwan, Could you consider @BillyONeal's suggestion?

@cd-pkwan
Copy link
Contributor Author

Yes, I will try to include @BillyONeal's suggestion.

Actually I have never used vcpkg. One day my client told me our software ChartDirector is available in vcpkg but the checksum is incorrect due to we updated our release file. That's why I am here. I will download vcpkg and try it myself to make sure any I would not break something by modifying the package script.

@BillyONeal
Copy link
Member

BillyONeal commented Apr 27, 2022

@cd-pkwan Thanks so much for helping fix the situation :D

vcpkg requires exact SHA matches to deliver on the "no changes means no changes" policy. Building the same code 5 years later should produce the same output.

@JonLiu1993
Copy link
Contributor

@cd-pkwan ,Can you please resolve the conflicts against master?

@cd-pkwan
Copy link
Contributor Author

I am embarassed to say that I am not familiar with vcpkg or github. I think the conflict is resolved, but there is still a checksum problem with versions/c-/chartdir.json. Can someone help to fix this problem as well?

@BillyONeal
Copy link
Member

The SHA listed there is the git-tree of the portfile directory. You can fix it with (assuming https://github.com/microsoft/vcpkg/ is your origin and your stuff is the current branch):

git merge origin/master
git checkout origin/master -- versions
.\vcpkg x-add-version chartdir
git commit -am "Update version database."

I just ran this for you.

@BillyONeal
Copy link
Member

I am embarassed to say that

No need to be embarrassed! In a perfect world we would give you one button to just fix this...

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.

You have modified or added at least one vcpkg.json where you should check the license field.

Details

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/chartdir/vcpkg.json

Valid values for the license field can be found in the documentation

@JonLiu1993 JonLiu1993 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jun 14, 2022
@BillyONeal BillyONeal merged commit e78aa84 into microsoft:master Jun 14, 2022
@BillyONeal
Copy link
Member

Thanks for the improvement!

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

[chartdir] update download link

4 participants