-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Rust test #20651
base: master
Are you sure you want to change the base?
Rust test #20651
Conversation
Posted a message about the breaking change in mingw-w64: https://sourceforge.net/p/mingw-w64/mailman/message/58760949/ For now the fix is only for MINGW64? Should it also target MINGW32 ? I will share the |
Where do we pull in mingw-w64? There is https://github.com/msys2/MSYS2-packages/blob/master/mingw-w64-cross-headers/PKGBUILD#L19 but it uses version 11.0.1 which does not contain the breaking change. |
More questions... Cheks are disabled in CI : https://github.com/msys2/MINGW-packages/blob/master/.ci/ci-build.sh#L136 What is the reason and would it make sense to enable them for specific packages like Rust ? |
they are disable as they can run very long or unexpectedly fail (which stops the whole build) |
wrong package, actually https://github.com/msys2/MINGW-packages/tree/master/mingw-w64-headers-git is used for non-msys environments |
I'm not sure about bothering with .gitignore - I don't remember seeing other packages doing that. Also, it might be a good idea to switch to the official upstream commits now that they work (https://github.com/rust-lang/compiler-builtins/commit/d8ab794ed61e2c7c0750f57332a680d5aa8db48c.patch and https://github.com/rust-lang/compiler-builtins/commit/3f47913bc6414bab4254d49f9f6e7238fecace69.patch) |
I also had errors in |
I can't reproduce anymore... So I am all set with this PR. |
Would be great to run the check in CI. I am only testing MINGW64 atm. |
maybe a separate job for build+check? |
Might be a worthwhile "discussion" or separate issue, but in the past having checks that failed but were 'expected' to fail (even if not configured as required for merging) would confuse contributors, and setting up the job so that the job itself would not fail when the step failed would make it too hard for people who cared to notice that it had failed. |
can this job be run by manual requests only? |
We could have a workflow_dispatch triggered workflow, but a) only users with rights to the repository could run it (others could enable and run it in their forks, but they could do that now by just taking I was just thinking, maybe some sort of hack such as some magic name of the head branch of the pull request could be a way for the creator of the pull request to signal that they want check to run? I don't know if there is any other convenient user-controlled flag that would be available to filter on. Maybe a label on the pull request, but I don't know that non-privileged users can label their own pull requests. |
I was not suggesting to check all packages but only selected ones. We could do something similar to what is done for installing or not. Alternatively we could move the rust tests to the build phase. |
I did a CI run on my fork with MINGW32 seems to have failed because of the
CLANG32 failed with It wouldn't surprise me a bit if some tests are screwed on CLANG32/CLANG64 due to the clang subsystem patch (that is, lying about the target being -gnu instead of -gnullvm) |
IIRC that's indeed because of using
Hard to tell, but likely caused by the same reason. |
Same failure on CLANG64 with gnullvm host/target
|
About the
The change that is mentioned is this one : mingw-w64/mingw-w64@a64c1f4 |
if the tests are expected to fail, it should pass, not fail, should it not? |
Regarding the dlltool tests on CLANG64: I suspect upstream they should have Unfortunately that doesn't help in our case where the target is a lie. I'm not sure how to work that out for this package - is there some way to specify a list of specific tests to ignore? |
There is documentation about testing : https://rustc-dev-guide.rust-lang.org/tests/running.html |
Skipping tests is dicussed here : rust-lang/rust#123342 (review) |
Interesting, I'll try that next |
Sorry, I meant tests that may pass or fail, we don't know, but won't generally hold up merging either way. But I think this would be better in a separate discussion. |
Oops, I led you astray - looks like those backtrace commits are included in rust 1.79.0 those 2 patches and their application can be removed now. |
No problems... Does it explain some of the test failures ? I too had a slightly hard time confirming if some of the commit I removed were really in 1.79.0. |
Unfortunately, no. This failed immediately when I was trying to build for clangarm64, with the patches failing to apply. These patches were only applied on arm64 anyway. |
The directory deletion error happens after the Rust build.
Could be related to the new rust-src package. |
I don't think so, it comes from a "test" directory, so it is probably just an issue cleaning up after the test. I've hit similar issues on CLANGARM64 which I assumed to be MAX_PATH issues.
That's from the "cleanup runner" powershell snippet trying to delete |
So we can't do much about it except asking Rust to shorten some of its paths. |
Found the Which makes the error strange...
The path does not look that long. We could list the content of EDIT: There is a new test that tries to create directories with non unicode path. |
It's needlessly long (probably that was the easiest way), might be worth filing a new issue upstream.
Sounds plausible, there were many issues with non unicode paths in MSYS2. |
If we can figure out what that path is (including whatever non-unicode characters) and reproduce it simply (without rust test suite), we could see if it's a bug in msys2-runtime or cygwin. |
The |
Yep, breaks for me: mkdir -p foo/$'\uD800'
rm -rf foo
rm: cannot remove 'foo': Directory not empty |
The tests\ui\issues\issue-2214.rs test fails with undefined reference to `__sinl_internal' and other math functions. Mateusz Mikuła analyzed the issue and found that the root cause is this change in mingw-w64: mingw-w64/mingw-w64@a64c1f4 The error happens because Rust pulls in lgamma from libmingwex.a, which pulls in sin from libmsvcrt.a, which in turn tries to pull in __sinl_internal from libmingwex.a and fails because of how Rust links MinGW libs. The proposed fix was to add an extra "-lmingwex" after the second "-lmsvcrt" in https://github.com/rust-lang/rust/blob/aa6a697a1c75b0aa06954136f7641706edadc2be/compiler/rustc_target/src/spec/base/windows_gnu.rs#L30.
This matches what was already being done for the split docs package.
* skip debuginfo tests on all platforms * skip tests that fail on CLANG* * enable no-fail-fast * checkdepends on gcc-compat package for clang Because of the bootstrapping hack, at least one test tries to use $TRIPLE-gcc to link rather than $TRIPLE-clang. The easiest workaround seems to be to ensure there is an alias to clang with that name.
by default makepkg is run with the --nocheck option this can be overriden per package by adding the package name to ./.ci/ci-check-list.txt
I don't understant the MINGW64 error:
It just says that some command did not execute successfully without giving any error message. |
That's the downside of
|
UPDATE: today they all fail. Don't know what is going on |
A bunch of crash tests were added in 1.79.0 including the one that fails (to crash). A later commit limited some the crash tests to only run on x86_64 and/or not on windows |
How do you fix something that is supposed to crash but does not ? |
We could probably skip all the tests from that directory as they aren't very useful for us. Upstream probably uses them to ease issue management. |
pull request telling them that there is no bug on windows :) |
The platform is irrelevant here, also you can see the compiler crashed as expected with CLANG64 env. |
ah maybe it is a llvm bug? Anyway it seems some rust tests aren't really tests but more like documentation of bugs |
No description provided.