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

[vcpkg-ci-openimageio] Add missing commas #39285

Merged

Conversation

JonLiu1993
Copy link
Member

Fixes #39276

vcpkg.json file in vcpkg/scripts/test_ports/vcpkg-ci-openimageio missing , character

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

@JonLiu1993 JonLiu1993 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Jun 14, 2024
@JonLiu1993 JonLiu1993 added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) and removed category:port-bug The issue is with a library, which is something the port should already support labels Jun 14, 2024
@talregev
Copy link
Contributor

Can we add functionality to: x-add-version command that also check the json format of testing ports, similar to what happened in regular ports?

@JonLiu1993
Copy link
Member Author

Can we add functionality to: x-add-version command that also check the json format of testing ports, similar to what happened in regular ports?

This is a good idea, I need to research it.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 14, 2024

Can we add functionality to: x-add-version command that also check the json format of testing ports, similar to what happened in regular ports?

This is a good idea, I need to research it.

The test ports are not in the versions registry, so x-add-version doesn't apply to them.
What does work for them, and should be part of PR checks, is format-manifest.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 14, 2024

But basically we should not need an extra check: vcpkg ci must report a port failure when the manifest is broken.

@talregev
Copy link
Contributor

talregev commented Jun 14, 2024

I know that tests ports is not the version registry, but x-add-version command do also format-manifest to the regular ports. So my suggestion is to do the same format-manifest on the same time to the tests ports.
You can add this check to the ci, but it better also to have it locally check, specially when this functionality already implement and already run locally on users that develops for vcpkg. It also gives you fast fix for run command that fix the format for you. this is can done only locally, and save precious ci time.

@WangWeiLin-MV
Copy link
Contributor

Try a time consuming command vcpkg x-ci-verify-versions --verify-git-trees.

See microsoft/vcpkg-tool/pull/1210

@dg0yt
Copy link
Contributor

dg0yt commented Jun 14, 2024

x-add-version ... this functionality already implement and already run locally on users that develops for vcpkg

The point is that developers have NO REASON to run x-add-version for overlay ports which are not in the versions registry.

Try a time consuming command vcpkg x-ci-verify-versions --verify-git-trees.

The point is that x-ci-verify-version has NO REASON to look at overlay ports which are not in the versions registry.

vcpkg ci must report a port failure when the manifest is broken.

This command MUST load the overlay ports, and it MUST report errors.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 14, 2024

From x64-windows CI logs, all about vcpkg-ci-openimageio is:

warning: vcpkg ci is an internal command which will change incompatibly or be removed at any time.
Creating failure logs output directory D:\a\_work\1\a\failure-logs.
warning: Errors occurred while parsing vcpkg-ci-openimageio.
warning: Use '--debug' to get more information about the parse failures.

We have vcpkg-ci-openimageio:x64-windows=pass in scripts/ci.baseline.txt so it MUST be a hard error when CI doesn't determine pass.

@talregev
Copy link
Contributor

talregev commented Jun 14, 2024

The point that the developer already run the x-add-version when they develop for regular ports. and it can add small amount of time check for json manifest in the test port. Also developers change sometimes the tests port. Also you want to have a fast fix command to fix the format for you, ratter to download and read the errors from the ci.

Also I am not against the ci check. It should be both.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 14, 2024

x-add-version does the wrong thing as the main effect (if it doesn't just fail).
x-format-manifest does the right thing as the main effect. I already call it when updating these ports. No change needed.

But this particular issue might have been introduced by editing files online in Github. There is no local interface to run vcpkg in that case. But there is CI.

@talregev
Copy link
Contributor

talregev commented Jun 14, 2024

x-add-version failed when there is a wrong format, and kindly hint you to run `vcpkg format-manifest '

There is no local interface to run vcpkg in that case. But there is CI.

That why I agree for both solutions. local and ci.

@talregev
Copy link
Contributor

@JonLiu1993 You can also run on linux:

find scripts/test_ports/ -name "vcpkg.json"  -exec ./vcpkg format-manifest "{}" \;

@dg0yt
Copy link
Contributor

dg0yt commented Jun 14, 2024

x-add-version failed when there is a wrong format, and kindly hint you to run vcpkg format-manifest

Not here:

> .\vcpkg.exe x-add-version --overlay-ports .\scripts\test_ports vcpkg-ci-openimageio   
error: can't load port vcpkg-ci-openimageio
C:\vcpkg\vcpkg\ports\vcpkg-ci-openimageio: error: vcpkg-ci-openimageio does not exist

x-add-version doesn't even find the overlay port... because the command is not meant to be used for overlay ports because no version records are maintained for overlay ports.

@talregev
Copy link
Contributor

talregev commented Jun 14, 2024

x-add-version failed when there is a wrong format, and kindly hint you to run vcpkg format-manifest

Not here:

> .\vcpkg.exe x-add-version --overlay-ports .\scripts\test_ports vcpkg-ci-openimageio   
error: can't load port vcpkg-ci-openimageio
C:\vcpkg\vcpkg\ports\vcpkg-ci-openimageio: error: vcpkg-ci-openimageio does not exist

x-add-version doesn't even find the overlay port... because the command is not meant to be used for overlay ports because no version records are maintained for overlay ports.

for testing port it should be partial functionality. It should not find the overly ports, just find the vcpkg.json files, and run on them the format-manifest that detected error without fixing it. and after it detect, it kindly suggest you to run vcpkg format-manifest on the wrong format json.
Similar to that command:

find scripts/test_ports/ -name "vcpkg.json" -exec ./vcpkg format-manifest "{}" \;

Yes, it needed new implementation. but the most of the implementation already there.

@talregev
Copy link
Contributor

@JonLiu1993 You can also run on linux:

find scripts/test_ports/ -name "vcpkg.json"  -exec ./vcpkg format-manifest "{}" \;

I can make a PR for that.

@JonLiu1993 JonLiu1993 marked this pull request as ready for review June 17, 2024 02:44
@JonLiu1993 JonLiu1993 added category:infrastructure Pertaining to the CI/Testing infrastrucutre and removed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) labels Jun 17, 2024
@WangWeiLin-MV WangWeiLin-MV added the info:reviewed Pull Request changes follow basic guidelines label Jun 17, 2024
@BillyONeal BillyONeal merged commit a885c80 into microsoft:master Jun 17, 2024
17 checks passed
@BillyONeal
Copy link
Member

Thanks!

@JonLiu1993 JonLiu1993 deleted the dev/Jon/vcpkg-ci-openimageio branch June 18, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vcpkg/scripts/test_ports/vcpkg-ci-openimageio/vcpkg.json is invalid
5 participants