Skip to content

[vcpkg-ci-llvm] Reduce llvm artifact to cacheable size#23896

Merged
strega-nil-ms merged 3 commits intomicrosoft:masterfrom
dg0yt:vcpkg-ci-llvm
Apr 7, 2022
Merged

[vcpkg-ci-llvm] Reduce llvm artifact to cacheable size#23896
strega-nil-ms merged 3 commits intomicrosoft:masterfrom
dg0yt:vcpkg-ci-llvm

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Mar 31, 2022

  • What does your PR fix?

    Cf. [vcpkg-ci] Current CI problems #21905 (comment):
    The llvm:x64-windows binary artifact couldn't be uploaded to binary caching. So it was rebuilt (2 h!) whenever needed, even for PRs which were unrelated to llvm but triggered vcpkg-ci-opencv:x64-windows.
    This is an attempt to mitigate the issue (until resolved in vcpkg tool) by removing some llvm features from windows CI.

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

    unchanged, no

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

    not needed

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 31, 2022

Triplet 773516e: baseline 40f3cbc: less targets 927d751: less flang 48a57c4
x86-windows 58 min 🟢 53 min 🟢 1.2 h 🟥 👎 1.3 h 🟢 👍
x64-windows 1.8 h 🟥 1.4 h 🟥 1.0 h 🟥 58 min 🟢 👍
x64-windows-static 1.5 h 🟥 1.4 h 🟥 1.4 h 🟥 60 min 🟢 👍
x64-windows-static-md 1.5 h 🟥 1.4 h 🟥 1.0 h 🟥 1.3 h 🟢 👍
x64-osx 2.8 h 🟢 2.6 h 🟢 cached 🆗 cached 🆗
x64-linux 2.1 h 🟥 2.0 h 🟥 1.5 h 🟥 1.3 h 🟥

🆗 Artifact reused from cache
🟢 Artifact upload successful
🟥 Artifact upload failed

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Apr 1, 2022
@JackBoosY
Copy link
Contributor

llvm always takes a lot of time and space.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

llvm always takes a lot of time and space.

This is not the point. The point is that there is an issue with uploading the artifact from CI to cache, and thus saving the build time the next time CI needs that artifact.

@JackBoosY
Copy link
Contributor

cc @BillyONeal, what do you think about this?

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

The issue was discussed in #21905 (comment) before. If someone can fix the uploads on the tool side, this PR isn't needed. Until this is done, I want a mitigation on the PR CI side.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

The change now seems to have the desired effect for windows triplets.

Note that this is needed mostly for x64-windows due to the chain
vcpkg-ci-opencv:x64-windows -> opencv[halide] -> halide -> llvm[clang,enable-rtti,tool].
llvm doesn't have many triggers, but vcpkg-ci-opencv has.
So if desired, we can scope this to x64-windows.

@dg0yt dg0yt marked this pull request as ready for review April 1, 2022 18:30
@Neumann-A
Copy link
Contributor

I feel like the underlying problem with the cache needs to be solved. If vcpkg detects that a blob will be to big it should just split it up.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

I feel like the underlying problem with the cache needs to be solved. If vcpkg detects that a blob will be to big it should just split it up.

Sure. But it was discussed and did move forward.

@cenit
Copy link
Contributor

cenit commented Apr 1, 2022

I feel like the underlying problem with the cache needs to be solved. If vcpkg detects that a blob will be to big it should just split it up.

many nuget servers also suffer a 2GB hard limit, and llvm already goes beyond that in many cases. Your suggestion cannot solve that issue, also because you have to consider when you have to fetch the tool: you don't know a priori if it's split and in how many parts

@cenit
Copy link
Contributor

cenit commented Apr 1, 2022

The change now seems to have the desired effect for windows triplets.

Note that this is needed mostly for x64-windows due to the chain

vcpkg-ci-opencv:x64-windows -> opencv[halide] -> halide -> llvm[clang,enable-rtti,tool].

llvm doesn't have many triggers, but vcpkg-ci-opencv has.

So if desired, we can scope this to x64-windows.

great job for now. It looks like only linux is still failing. Is it so big the final artifact? or do you have any idea why is it failing?

@Neumann-A
Copy link
Contributor

you don't know a priori if it's split and in how many parts

just look for <hash>-<part> first before looking just for <hash>. The amount of parts could be either determined by the size of <part> (e.g. always splitting to like 2 GB and if it is lower you are finished) or just by checking if a next part exist.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

you don't know a priori if it's split and in how many parts

Well there is at least one natural line (but not 50/50): Split the debug part (huge) from the release part. In a perfect world, the release part would be valid also for release-only configs.

It looks like only linux is still failing. Is it so big the final artifact?

In the past users had issues with caching of gdal[tools]:x64-linux. Well, statically linked every tiny tool had all the debug symbols in the debug variant... We solved it by no longer build the tools for debug.

or do you have any idea why is it failing?

It is failing for wanting to send too much data:
Error: curl failed to put file to https://vcpkgbinarycache.blob.core.windows.net/cache/12dec1eaa642a1018bd8b451066369a509eca9771b1367199b0d930a296e8953.zip?*** SECRET *** with exit code '0' and http code '413'
Code 413 is Payload too large.

And similiar to the gdal:x64-linux example, llvm always builds with static linkage.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

Stop discuss the splitting here. This PR is meant as a temporary mitigation.

@JackBoosY JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Apr 2, 2022
@JackBoosY
Copy link
Contributor

I'm not sure about this part, hoping other member review this PR.

@strega-nil-ms
Copy link
Contributor

Adding this to the Thursday meeting; I'm a soft approve.

@strega-nil-ms strega-nil-ms added requires:discussion and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Apr 5, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 7, 2022

Adding this to the Thursday meeting; I'm a soft approve.

🙏
It affects every second PR I make: libwebp, zziplib, ... It really matters.

@strega-nil-ms
Copy link
Contributor

Thanks @dg0yt! We are good with this PR :)

@strega-nil-ms strega-nil-ms merged commit 70fafdf into microsoft:master Apr 7, 2022
@dg0yt dg0yt deleted the vcpkg-ci-llvm branch April 7, 2022 19:52
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants