This is an archive of the discontinued LLVM Phabricator instance.

[ClangDepScan] Add UNSUPPORTED: system-windows to failing P1689.cppm test
ClosedPublic

Authored by TWeaver on Feb 10 2023, 8:51 AM.

Download Raw Diff

Details

Reviewers

thakis
ChuanqiXu
probinson

Summary

This test has caused numerous windows bot failures:

https://lab.llvm.org/buildbot/#/builders/216/builds/17032

The test was added in https://reviews.llvm.org/D137527

The omission of this test from windows builds was reverted in https://reviews.llvm.org/D139168

Diff Detail

Event Timeline

TWeaver created this revision.Feb 10 2023, 8:51 AM

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 8:51 AM

Herald added subscribers: kadircet, arphaman. · View Herald Transcript

TWeaver requested review of this revision.Feb 10 2023, 8:51 AM

Herald added subscribers: MaskRay, ilya-biryukov. · View Herald TranscriptFeb 10 2023, 8:51 AM

TWeaver mentioned this in D137527: [C++20] [Modules] [ClangScanDeps] Add ClangScanDeps support for C++20 Named Modules in P1689 format (2/4).Feb 10 2023, 8:52 AM

TWeaver edited the summary of this revision. (Show Details)Feb 10 2023, 8:55 AM

Let's fix the test or revert the change instead.

slight confusion on the test name, sorted now

@thakis that's a great idea in theory, but this has been caused by the reversion of 4/4 in this series of patches, the test was added in 2/4, so wouldn't there potentially a need to revert at least 2/4, 3/4 and 4/4, which could cause further errors/failures if 1/4 is malformed.

But I'm happy to concede and wait it out for a fix.

dyung added a subscriber: dyung.Feb 10 2023, 9:12 AM

Reverts are cheap, we can just revert the last 3.

Going to revert:

2/4 : https://reviews.llvm.org/D137527
3/4 : https://reviews.llvm.org/D137534

instead.

Harbormaster completed remote builds in B213079: Diff 496505.Feb 10 2023, 10:12 AM

There is the potential for further fall out from reverting these additional patches. I strongly feel that the best course of action here is to add the UNSUPPORTED line outlined in this patch and push this issue back onto the original author who's responsible for reverting the patch that causes this to fail.

There are several build bots failing as a result of the revert in https://reviews.llvm.org/D139168 and it's important to get those green and enable failure reporting for future incoming patches.

The original author can then address the Windows failures in this test in future, in their own time.

LGTM. A revert isn't cheap when it reverts a test to a broken state.

This revision is now accepted and ready to land.Feb 10 2023, 10:24 AM

Committed in https://reviews.llvm.org/rG79a3803bb2ccdd852436cd1653017a1159a12157

My apologies for missing the review link in the commit message.

ChuanqiXu mentioned this in D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4).Feb 10 2023, 8:51 PM

Revision Contents

Path

Size

clang/

test/

ClangScanDeps/

P1689.cppm

2 lines

Diff 496505

clang/test/ClangScanDeps/P1689.cppm

				// UNSUPPORTED: system-windows

	// RUN: rm -fr %t			// RUN: rm -fr %t
	// RUN: mkdir -p %t			// RUN: mkdir -p %t
	// RUN: split-file %s %t			// RUN: split-file %s %t
	//			//
	// RUN: sed "s\|DIR\|%/t\|g" %t/P1689.json.in > %t/P1689.json			// RUN: sed "s\|DIR\|%/t\|g" %t/P1689.json.in > %t/P1689.json
	// RUN: clang-scan-deps -compilation-database %t/P1689.json -format=p1689 \| FileCheck %t/Checks.cpp -DPREFIX=%/t			// RUN: clang-scan-deps -compilation-database %t/P1689.json -format=p1689 \| FileCheck %t/Checks.cpp -DPREFIX=%/t
	// RUN: clang-scan-deps --mode=preprocess-dependency-directives -compilation-database %t/P1689.json -format=p1689 \| FileCheck %t/Checks.cpp -DPREFIX=%/t			// RUN: clang-scan-deps --mode=preprocess-dependency-directives -compilation-database %t/P1689.json -format=p1689 \| FileCheck %t/Checks.cpp -DPREFIX=%/t
	//			//
	▲ Show 20 Lines • Show All 270 Lines • Show Last 20 Lines