Skip to content

Conversation

BillyONeal
Copy link
Member

This changes our PR builds to treat 'fail' in the ci.baseline.txt as 'skip' instead of using tombstones.

We currently have large numbers of spurious failures that get enshrined in PRs through no fault of a PR author, removing the tombstones concept will fix those by allowing the user to retry. This does mean we accept some risk of not detecting when a port is 'fixed', but that failure is reasonable for us to handle after we see it in CI, but that seems worth it given that it lets us get rid of the tombstone concept.

This also helps out the binary caching feature, because we don't have to figure out how to productize tombstones.

@BillyONeal BillyONeal added info:internal category:infrastructure Pertaining to the CI/Testing infrastrucutre labels Jul 1, 2020
@BillyONeal BillyONeal marked this pull request as draft July 1, 2020 20:20
@BillyONeal
Copy link
Member Author

I forgot about our 'save_failure_logs' function which will be broken by this, switching to draft.

Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

I think this is a really good change.

@PhoebeHui PhoebeHui self-assigned this Jul 2, 2020
This changes our PR builds to treat 'fail' in the ci.baseline.txt as 'skip' instead of using tombstones.

We currently have large numbers of spurious failures that get enshrined in PRs through no fault of a PR author, removing the tombstones concept will fix those by allowing the user to retry. This does mean we accept some risk of not detecting when a port is 'fixed', but that failure is reasonable for us to handle after we see it in CI, but that seems worth it given that it lets us get rid of the tombstone concept.

This also helps out the binary caching feature, because we don't have to figure out how to productize tombstones.
@BillyONeal BillyONeal force-pushed the remove_tombstones branch 3 times, most recently from c39d715 to 78d82f2 Compare July 2, 2020 04:35
@BillyONeal
Copy link
Member Author

@BillyONeal BillyONeal force-pushed the remove_tombstones branch from 317f68c to 8cd3f8f Compare July 2, 2020 05:05
@BillyONeal BillyONeal marked this pull request as ready for review July 2, 2020 05:16
@BillyONeal BillyONeal requested a review from strega-nil July 2, 2020 05:16
@BillyONeal BillyONeal force-pushed the remove_tombstones branch from 6c95633 to 129f482 Compare July 2, 2020 05:56
@BillyONeal BillyONeal changed the title [vcpkg] Remove the tombstones concept. [vcpkg] Remove the tombstones and 'ignore' baseline concepts. Jul 2, 2020
@@ -1,4 +1,5 @@
#pragma once
#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double pragma!

fs.create_directories(paths.buildtrees / name, ec);
const auto abi_file_path = paths.buildtrees / name / (triplet.canonical_name() + ".vcpkg_abi_info.txt");
auto current_build_tree = paths.build_dir(action.spec);
fs.create_directory(current_build_tree, ec);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use ignore_errors here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is paths.buildtrees guaranteed to exist at this point? Should this be create_directories()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm or maybe it should be VCPKG_LINE_INFO. No reason to continue if we can't create a build tree.

I was trying to avoid create_directories because in profiling that showed up as being expensive. Let me see if I can find where we create buildtrees (because we do, it worked in testing so something must have done it already...)

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like we create buildtrees when we make the "CMakeVarProvider"

{
using namespace vcpkg::Build;

const fs::path log_path = fs::u8path(".log");
Copy link
Collaborator

Choose a reason for hiding this comment

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

log_extension or log_ext? I confused log_path with "prefix path under which logs are found"

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to 'dot_log' and 'readme_dot_log' respectively.

@BillyONeal BillyONeal merged commit a28bfe7 into microsoft:master Jul 3, 2020
@BillyONeal BillyONeal deleted the remove_tombstones branch July 3, 2020 03:20
E452003 added a commit to E452003/vcpkg that referenced this pull request Jul 6, 2020
* 'master' of https://github.com/microsoft/vcpkg: (1418 commits)
  [vcpkg integrate] Clean up vcpkg.target file (microsoft#4608)
  [vcpkg_from_sourceforge] Add retry mirror function (2/2) (microsoft#12018)
  [pcre2] Restore the https://ftp.pcre.org/ mirror in addition to the SourceForge mirrors. (microsoft#12233)
  [xercesc] rename feature from xmlch_wchar to xmlch-wchar (microsoft#12205)
  [safeint] Update to 3.24 (microsoft#12217)
  [vcpkg] Remove the tombstones and 'ignore' baseline concepts. (microsoft#12197)
  [msbuild] Revert the importance to Normal (microsoft#12212)
  [vtk] Added opengl feature. (microsoft#11399)
  [span-lite] Update to 0.7.0 (microsoft#12206)
  [cmocka libarchive libiconv libpq libxml2 plibsys] fix drive-by error in vcpkg-cmake-wrappers (microsoft#12196)
  [azure-iot-sdk-c] Fix feature name and enable to build (microsoft#12209)
  [vcpkg] Improve performance of compiler tracking by suppressing aspects of CMake's compiler detection. (microsoft#12203)
  [vcpkg] Remove all uses of Foo::Foo() noexcept = default; to fix microsoft#9955 (microsoft#12201)
  [sqlite3] update to 3.32.3 to deal with security issues (microsoft#12185)
  [infoware] Bump version to 0.5.4 (microsoft#12167)
  [imgui] Update to 1.77 (microsoft#12155)
  [vcpkg] Update message in bootstrap.ps1 (microsoft#12145)
  [vcpkg] Enable NuGet-based binary caching via mono (microsoft#12170)
  Don't change manifest root when manifest isn't enabled. (microsoft#12191)
  Fix sourceparagraph:BooleanField (microsoft#12192)
  ...
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
…oft#12197)

This changes our PR builds to treat 'fail' in the ci.baseline.txt as 'skip' instead of using tombstones.

We currently have large numbers of spurious failures that get enshrined in PRs through no fault of a PR author, removing the tombstones concept will fix those by allowing the user to retry. This does mean we accept some risk of not detecting when a port is 'fixed', but that failure is reasonable for us to handle after we see it in CI, but that seems worth it given that it lets us get rid of the tombstone concept.

This also helps out the binary caching feature, because we don't have to figure out how to productize tombstones.
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