Skip to content

[ci] Remove unaffected ports from pull request builds#21078

Merged
dan-shaw merged 4 commits intomicrosoft:masterfrom
dg0yt:parent-hashes
Nov 1, 2021
Merged

[ci] Remove unaffected ports from pull request builds#21078
dan-shaw merged 4 commits intomicrosoft:masterfrom
dg0yt:parent-hashes

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Oct 30, 2021

  • What does your PR fix?

    Limits the impact of the cache state of vcpkg master on pull requests. The original state leads to a waste of build resources for pull request CI runs triggered shortly after significant changes to master. Cf. Add option to remove unaffected ports from ci action vcpkg-tool#210.

    Updates the documentation for the binary caching effect of the BuildReason parameter of the test script.

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

    all, no

  • Does your PR follow the maintainer guide?

    --

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

    --

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 30, 2021

Test CI run with modified port gsl-lite available at https://dev.azure.com/vcpkg/public/_build/results?buildId=62203&view=results
Installs 55 ports on x64-windows, re-builds gsl-lite and depending ports as expected.
Additional output after the ci clean step:

2021-10-30T13:10:15.5044706Z A suitable version of cmake was not found (required v3.21.1). Downloading portable cmake v3.21.1...
2021-10-30T13:10:15.5051367Z Downloading cmake...
2021-10-30T13:10:15.5058771Z   https://github.com/Kitware/CMake/releases/download/v3.21.1/cmake-3.21.1-windows-i386.zip -> D:\downloads\cmake-3.21.1-windows-i386.zip
2021-10-30T13:10:15.5065958Z Extracting cmake...
2021-10-30T13:10:15.5074341Z A suitable version of 7zip was not found (required v19.0.0). Downloading portable 7zip v19.0.0...
2021-10-30T13:10:15.5081281Z Downloading 7zip...
2021-10-30T13:10:15.5088301Z   https://www.7-zip.org/a/7z1900-x64.msi -> D:\downloads\7z1900-x64.msi
2021-10-30T13:10:15.5094992Z Extracting 7zip...
2021-10-30T13:10:17.9948888Z D:\downloads\tools\cmake-3.21.1-windows\cmake-3.21.1-windows-i386\bin\cmake.exe
2021-10-30T13:10:18.1201805Z A suitable version of ninja was not found (required v1.10.2). Downloading portable ninja v1.10.2...
2021-10-30T13:10:18.1209346Z Downloading ninja...
2021-10-30T13:10:18.1217064Z   https://github.com/ninja-build/ninja/releases/download/v1.10.2/ninja-win.zip -> D:\downloads\ninja-win-1.10.2.zip
2021-10-30T13:10:18.1224184Z Extracting ninja...
2021-10-30T13:10:18.1484378Z D:\downloads\tools\ninja-1.10.2-windows\ninja.exe
2021-10-30T13:10:19.2650005Z A suitable version of git was not found (required v2.32.0). Downloading portable git v2.32.0...
2021-10-30T13:10:19.2657294Z Downloading git...
2021-10-30T13:10:19.2665123Z   https://github.com/git-for-windows/git/releases/download/v2.32.0.windows.2/PortableGit-2.32.0.2-32-bit.7z.exe -> D:\downloads\PortableGit-2.32.0.2-32-bit.7z.exe
2021-10-30T13:10:19.2672472Z Extracting git...
2021-10-30T13:10:22.3417531Z D:\downloads\tools\git-2.32.0.2-windows\mingw32\bin\git.exe
2021-10-30T13:10:22.3496155Z Determining parent hashes using HEAD~1
2021-10-30T13:10:24.1374050Z Loading dependency information for 158 packages...
2021-10-30T13:10:24.2388681Z Loading dependency information for 410 packages...
2021-10-30T13:10:27.8530544Z Time to determine pass/fail: 4.107 s
2021-10-30T13:10:28.5418159Z Running CI using parent hashes

The tools are fetched explicitly before the parent-hashes step, to unclutter the output and facilitate output filtering.

I remove the test port modification now, and then it is ready for review.

@dg0yt
Copy link
Contributor Author

dg0yt commented Oct 30, 2021

Switched to git revert instead of git checkout -- because the latter didn't remove added files which affects hashes.

@JackBoosY JackBoosY requested a review from BillyONeal November 1, 2021 02:17
@JackBoosY JackBoosY added the depends:vm-update PR contains changes to the VM provisioning scripts label Nov 1, 2021
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 1, 2021

@JackBoosY This does not need a "vm-update". vcpkg is bootstrapped, not provisioned.

@JackBoosY JackBoosY added category:infrastructure Pertaining to the CI/Testing infrastrucutre and removed depends:vm-update PR contains changes to the VM provisioning scripts labels Nov 1, 2021
@JackBoosY
Copy link
Contributor

Tagged wrong label.

# Prefetch tools for better output
& "./vcpkg$executableExtension" fetch cmake
& "./vcpkg$executableExtension" fetch ninja
& "./vcpkg$executableExtension" fetch git
Copy link
Member

Choose a reason for hiding this comment

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

Technically this git and the one used on line 184 are not the same git. Our UX for fetch is... not great :)

Copy link
Member

Choose a reason for hiding this comment

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

(But this doesn't matter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is also what I thought.
What we might do on another revision is moving the fetches out of the script, next to the bootstrap step.

@dan-shaw dan-shaw merged commit eea0238 into microsoft:master Nov 1, 2021
@BillyONeal
Copy link
Member

@dg0yt Thank you so much for resolving this years-long-outstanding issue with our validation pipeline! 🥳🥳🥳🥳🥳🎉🎉🎉🎉🎉🎊🎊🎊🎊🎊🥳🥳🥳🥳🥳

@BillyONeal
Copy link
Member

BillyONeal commented Nov 3, 2021

Initial results from the first day of this are promising. If we see this pattern for a week or so we can look at turning on #15983

image

(The blue and light blue are VM costs, the rest is bandwidth and storage. You can see we spend almost nothing on storage...)

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 3, 2021

We still need to see some major changes to master plus pull request activity. It happens app. twice a month. For a perfect storm, merge some key script audit, and than update some pending audit PRs.

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 3, 2021

Perhaps you need to track average cost per CI run, not total cost. For me, the current bottleneck is review, not CI.

@BillyONeal
Copy link
Member

Perhaps you need to track average cost per CI run, not total cost. For me, the current bottleneck is review, not CI.

Total cost is the only thing we get yelled at for.

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 3, 2021

Total cost is the only thing we get yelled at for.

Sure! But

Total cost = Number of activities * Cost per activity

and you cannot say this PR improved the situation after we just saw a period where the Number of activities was low.

@BillyONeal
Copy link
Member

Of course! And that's why I said "looks promising" rather than "I know this will work"

@cenit
Copy link
Contributor

cenit commented Nov 3, 2021

#21058 this will trigger a full rebuild and is a blocking issue for me right now :)

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 3, 2021

For a perfect storm, merge some key script audit, and than update some pending audit PRs.

Oops, I should have said "and than update some tiny leaf PR". Updating PRs with large impact will continue to need expensive CI runs. But anyway, it looks like PRs really do not build more than necessary for that PR after the recent merge to master.

@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 12, 2021

We still need to see some major changes to master plus pull request activity. It happens app. twice a month. For a perfect storm, merge some key script audit, and than update some pending audit PRs.

The other perfect storm was breaking a particular package. In the past, this would have blocked all CI builds, due to the cache miss. Hence this PR's title.
Now, the #21173 vcpkg_find_acquire_program change breaking fontconfig proved that this PR's mission was achieved: PR builds which don't need fontconfig are not broken.

@cenit
Copy link
Contributor

cenit commented Nov 12, 2021

great job, @dg0yt

@dg0yt dg0yt deleted the parent-hashes branch November 13, 2021 04:45
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants