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

Replace most call to grep in run-make by a script that cat the input. #46207

Merged
merged 2 commits into from
Nov 29, 2017

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Nov 23, 2017

Introduced a new src/etc/cat-and-grep.sh script (called in run-make as $(CGREP)), which prints the input and do a grep simultaneously. This is mainly used to debug spurious failures in run-make, such as the spurious error in #45810, as well as real errors such as #46126.

(cc #40713)

Some grep still remains, mainly the grep -c calls that count the number of matches and print the result to stdout.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 23, 2017
@bors
Copy link
Contributor

bors commented Nov 24, 2017

☔ The latest upstream changes (presumably #45946) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Nov 25, 2017

☔ The latest upstream changes (presumably #46008) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Nice! My only hesitation is that this script is probably pretty likely to grow a lot over time and receive and endless stream of "be compatible with python 2 and python 3" PRs along with "pep8 style" PRs. I wonder if we could reduce the maintenance burden on ourselves somewhat with a shell pipeline using tee and grep or something like that?

@kennytm
Copy link
Member Author

kennytm commented Nov 25, 2017

@alexcrichton Done (now hope that there isn't an endless stream of "be compatible with /bin/sh" 😆).

@alexcrichton
Copy link
Member

@bors: r+

Heh, well we always seem to get an endless stream of something, no? Thanks!

@bors
Copy link
Contributor

bors commented Nov 26, 2017

📌 Commit b18134a has been approved by alexcrichton

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2017
@kennytm
Copy link
Member Author

kennytm commented Nov 26, 2017

Note: I've temporarily re-enabled the leak sanitizer test disabled from #46126. This will be re-disabled after we've got the output. Please don't roll this PR up.

Since this PR is in the 3rd place of the actual queue, let me test using the PR CI instead.

Edit: Well, just no output from the sanitizer test 🤷. https://travis-ci.org/rust-lang/rust/jobs/307521898#L6024

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 26, 2017
@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 26, 2017
@kennytm
Copy link
Member Author

kennytm commented Nov 26, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Nov 26, 2017

📌 Commit d8701a7 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 27, 2017

⌛ Testing commit d8701a73df86cdf4bb77a6623f068f0492ba4cac with merge f7b27b10694b71739b4c2c3bd0aba373fdcf5f3e...

@bors
Copy link
Contributor

bors commented Nov 27, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member Author

kennytm commented Nov 27, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Nov 27, 2017

📌 Commit 2ed7cce has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 27, 2017

⌛ Testing commit 2ed7cce7ddd3bf0f10f850ea19d2709ca3811a03 with merge 2a7a34495a1c483b8b526b0536c7693adc27b8e9...

@bors
Copy link
Contributor

bors commented Nov 27, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Nov 27, 2017

@bors retry

Looks like spurious. Good thing is we've got the output (mmap error?).

@bors
Copy link
Contributor

bors commented Nov 28, 2017

⌛ Testing commit 2ed7cce7ddd3bf0f10f850ea19d2709ca3811a03 with merge 03253592babb2bbd39de96121f4a6983988937d5...

@bors
Copy link
Contributor

bors commented Nov 28, 2017

💔 Test failed - status-appveyor

@kennytm
Copy link
Member Author

kennytm commented Nov 28, 2017

The error is legit... which is because the cdylib-fewer-symbols test was broken.

[01:51:45] ---- [run-make] run-make\cdylib-fewer-symbols stdout ----
[01:51:45] 	
[01:51:45] error: make failed
[01:51:45] status: exit code: 2
[01:51:45] command: "make"
[01:51:45] stdout:
[01:51:45] ------------------------------------------
[01:51:45] PATH="/c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/cdylib-fewer-symbols.stage2-x86_64-pc-windows-gnu:C:\projects\rust\build\x86_64-pc-windows-gnu\stage2\bin:/c/projects/rust/build/x86_64-pc-windows-gnu/stage0-tools/x86_64-pc-windows-gnu/release/deps:/c/projects/rust/build/x86_64-pc-windows-gnu/stage0-sysroot/lib/rustlib/x86_64-pc-windows-gnu/lib:/c/Program Files (x86)/Inno Setup 5:/c/Python27:/c/projects/rust/mingw64/bin:/usr/bin:/c/Perl/site/bin:/c/Perl/bin:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0:/c/Program Files/7-Zip:/c/Program Files/Microsoft/Web Platform Installer:/c/Tools/GitVersion:/c/Tools/PsTools:/c/Program Files/Git LFS:/c/Program Files (x86)/Subversion/bin:/c/Program Files/Microsoft SQL Server/120/Tools/Binn:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/110/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/Tools/Binn:/c/Program Files/Microsoft SQL Server/120/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/Tools/Binn/ManagementStudio:/c/Tools/WebDriver:/c/Program Files (x86)/Microsoft SDKs/TypeScript/1.4:/c/Program Files (x86)/Microsoft Visual Studio 12.0/Common7/IDE/PrivateAssemblies:/c/Program Files (x86)/Microsoft SDKs/Azure/CLI/wbin:/c/Ruby193/bin:/c/Tools/NUnit/bin:/c/Tools/xUnit:/c/Tools/MSpec:/c/Tools/Coverity/bin:/c/Program Files (x86)/CMake/bin:/c/go/bin:/c/Program Files/Java/jdk1.8.0/bin:/c/Python27:/c/Program Files/nodejs:/c/Program Files (x86)/iojs:/c/Program Files/iojs:/c/Users/appveyor/AppData/Roaming/npm:/c/Program Files/Microsoft SQL Server/130/Tools/Binn:/c/Program Files (x86)/MSBuild/14.0/Bin:/c/Tools/NuGet:/c/Program Files (x86)/Microsoft Visual Studio 14.0/Common7/IDE/CommonExtensions/Microsoft/TestWindow:/c/Program Files/Microsoft DNX/Dnvm:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/130/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/130/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/130/DTS/Binn:/c/Program Files/Microsoft SQL Server/130/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/110/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/DTS/Binn:/c/Program Files (x86)/Apache/Maven/bin:/c/Python27/Scripts:/c/Tools/NUnit3:/c/Program Files/Mercurial:/c/Program Files/LLVM/bin:/c/Program Files/dotnet:/c/Program Files/erl8.3/bin:/c/Tools/curl/bin:/c/Program Files/Amazon/AWSCLI:/c/Program Files (x86)/Microsoft SQL Server/140/DTS/Binn:/c/Program Files (x86)/Microsoft Visual Studio 14.0/Common7/IDE/Extensions/Microsoft/SQLDB/DAC/140:/c/Program Files/Git/cmd:/c/Program Files/Git/usr/bin:/c/Tools/vcpkg:/c/Program Files/Microsoft Service Fabric/bin/Fabric/Fabric.Code:/c/Program Files/Microsoft SDKs/Service Fabric/Tools/ServiceFabricLocalClusterManager:/c/Program Files (x86)/Yarn/bin:/c/ProgramData/chocolatey/bin:/c/Program Files (x86)/Microsoft SQL Server/140/Tools/Binn:/c/Program Files/Microsoft SQL Server/140/Tools/Binn:/c/Program Files/Microsoft SQL Server/140/DTS/Binn:/c/Program Files (x86)/nodejs:/c/Users/appveyor/AppData/Local/Yarn/bin:/c/Users/appveyor/AppData/Roaming/npm:/c/Program Files/AppVeyor/BuildAgent:/c/projects/rust:/c/projects/rust/handle" 'C:\projects\rust\build\x86_64-pc-windows-gnu\stage2\bin\rustc.exe' --out-dir /c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/cdylib-fewer-symbols.stage2-x86_64-pc-windows-gnu -L /c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/cdylib-fewer-symbols.stage2-x86_64-pc-windows-gnu  foo.rs
[01:51:45] nm -g "/c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/cdylib-fewer-symbols.stage2-x86_64-pc-windows-gnu/foo.dll" | "C:\projects\rust/src/etc/cat-and-grep.sh" -v __rdl_ __rde_ __rg_ __rust_
[01:51:45] [[[ begin stdout ]]]
[01:51:45]                  U .bss
...
<snip>
...
[01:51:45] 0000000066e38a38 B __onexitend
[01:51:45] 0000000066d83d30 T __rdl_alloc
[01:51:45] 0000000066d83f90 T __rdl_alloc_excess
[01:51:45] 0000000066d83f00 T __rdl_alloc_zeroed
[01:51:45] 0000000066d83df0 T __rdl_dealloc
[01:51:45] 0000000066d840a0 T __rdl_grow_in_place
[01:51:45] 0000000066d83dc0 T __rdl_oom
[01:51:45] 0000000066d83e10 T __rdl_realloc
[01:51:45] 0000000066d84000 T __rdl_realloc_excess
[01:51:45] 0000000066d84120 T __rdl_shrink_in_place
[01:51:45] 0000000066d83e00 T __rdl_usable_size
[01:51:45] 0000000066deee00 T __report_gsfailure
[01:51:45] 0000000066e21140 R __rt_psrelocs_end
[01:51:45] 0000000000000000 A __rt_psrelocs_size
[01:51:45] 0000000066e21140 R __rt_psrelocs_start
[01:51:45] 0000000066e21140 R __RUNTIME_PSEUDO_RELOC_LIST__
[01:51:45] 0000000066e21140 R __RUNTIME_PSEUDO_RELOC_LIST_END__
[01:51:45] 0000000066d81430 T __rust_alloc
[01:51:45] 0000000066d814a0 T __rust_alloc_excess
[01:51:45] 0000000066d81490 T __rust_alloc_zeroed
[01:51:45] 0000000066d81450 T __rust_dealloc
[01:51:45] 0000000066d814e0 T __rust_grow_in_place
[01:51:45] 0000000066dc9fc0 T __rust_maybe_catch_panic
[01:51:45] 0000000066d81440 T __rust_oom
[01:51:45] 0000000066d81470 T __rust_realloc
[01:51:45] 0000000066d814b0 T __rust_realloc_excess
[01:51:45] 0000000066d814f0 T __rust_shrink_in_place
[01:51:45] 0000000066dca010 T __rust_start_panic
[01:51:45] 0000000066d81460 T __rust_usable_size
[01:51:45] 0000000000001000 A __section_alignment__
...
<snip>
...
[01:51:45] 0000000066deeae4 T WSAStartup
[01:51:45] 
[01:51:45] [[[ end stdout ]]]
[01:51:45] Error: should not match: __rdl_
[01:51:45] Error: should not match: __rust_
[01:51:45] 
[01:51:45] ------------------------------------------
[01:51:45] stderr:
[01:51:45] ------------------------------------------
[01:51:45] make: *** [Makefile:12: all] Error 1
[01:51:45] 
[01:51:45] ------------------------------------------
[01:51:45] 
[01:51:45] thread '[run-make] run-make\cdylib-fewer-symbols' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:2570:8
[01:51:45] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:51:45] 
[01:51:45] 
[01:51:45] failures:
[01:51:45]     [run-make] run-make\cdylib-fewer-symbols
[01:51:45] 
[01:51:45] test result: FAILED. 167 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[01:51:45] 
[01:51:45] thread 'main' panicked at 'Some tests failed', src\tools\compiletest\src\main.rs:331:21

We can clearly see the __rdl_ and __rust_ symbols still remain in the DLL. The changeset introduced in this PR about the test is:

diff --git a/src/test/run-make/cdylib-fewer-symbols/Makefile b/src/test/run-make/cdylib-fewer-symbols/Makefile
index 954ee792460a..929d5571194b 100644
--- a/src/test/run-make/cdylib-fewer-symbols/Makefile
+++ b/src/test/run-make/cdylib-fewer-symbols/Makefile
@@ -9,9 +9,5 @@ all:
 else
 all:
 	$(RUSTC) foo.rs
-	nm -g "$(call DYLIB,foo)"
-	nm -g "$(call DYLIB,foo)" | grep -vq __rdl_
-	nm -g "$(call DYLIB,foo)" | grep -vq __rde_
-	nm -g "$(call DYLIB,foo)" | grep -vq __rg_
-	nm -g "$(call DYLIB,foo)" | grep -vq __rust_
+	nm -g "$(call DYLIB,foo)" | $(CGREP) -v __rdl_ __rde_ __rg_ __rust_

The problem is that grep -vq __rdl_ will return success (exit code 0) as long as any single line does not contain __rdl_, basically meaning the whole test is bogus, it just always succeeds!

cc #45710 @alexcrichton — I'm going to disable the test for all Windows (it is already disabled for MSVC), is this okay?

Introduced a new src/etc/cat-and-grep.sh script (called in run-make as
$(CGREP)), which prints the input and do a grep simultaneously. This is
mainly used to debug spurious failures in run-make, such as the sanitizer
error in rust-lang#45810, as well as real errors such as rust-lang#46126.
@alexcrichton
Copy link
Member

Gah sorry about that! Yes feel free to disable and I will investigate when I can

@kennytm
Copy link
Member Author

kennytm commented Nov 28, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Nov 28, 2017

📌 Commit 918158d has been approved by alexcrichton

bors added a commit that referenced this pull request Nov 28, 2017
Replace most call to grep in run-make by a script that cat the input.

Introduced a new `src/etc/cat-and-grep.sh` script (called in run-make as `$(CGREP)`), which prints the input and do a grep simultaneously. This is mainly used to debug spurious failures in run-make, such as the spurious error in #45810, as well as real errors such as #46126.

(cc #40713)

Some `grep` still remains, mainly the `grep -c` calls that count the number of matches and print the result to stdout.
@bors
Copy link
Contributor

bors commented Nov 28, 2017

⌛ Testing commit 918158d with merge 77ab3a1...

@bors
Copy link
Contributor

bors commented Nov 29, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 77ab3a1 to master...

@bors bors merged commit 918158d into rust-lang:master Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants