rustc: disable fragile tests on darwin #21741
Conversation
rustc-1.14.0 builds are failing on hydra due to test failures of tcp functionality that do not look like significant failures. This patch causes these functions to be ignored on macos.
|
@acowley, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vcunat, @the-kenny and @LnL7 to be potential reviewers. |
|
I have also seen this issue with a hydra jobset and building it locally worked fine, I assume this is an unstable test. |
| optionalString stdenv.isDarwin '' | ||
| substituteInPlace mk/rt.mk --replace "_osx" "_10.4" | ||
| sed -z -e 's/#\[test\]\n[[:space:]]*fn timeouts()/#[cfg_attr(target_os = "macos", ignore)]\n #[test]\n fn timeouts()/' \ |
There was a problem hiding this comment.
I think this sed-approach is too fragile. Can you please generate a .patchfile we can apply via patches?
There was a problem hiding this comment.
You can do this by cloning rustc, making the relevant changes, then committing and running git format-patch @^
There was a problem hiding this comment.
I disagree that it's too fragile. It is robust to substantial changes to the file. The only way it stops helping is if the developers' convention for defining tests changes.
There was a problem hiding this comment.
The problem is that it will silently break as soon as the sed doesn't match anymore. At least with a .patch we will get a build error instead of sporadic failures because it doesn't apply cleanly anymore. Also note that there are already other tests we disable via this mechanism.
If you really don't want to create a patchfile, please use substituteInPlace instead of sed as it's more readable (as suggested in https://nixos.org/nixpkgs/manual/#ssec-stdenv-functions) and more commonly used in nixpkgs.
Also, there's no need to use the cfg_attr(target_os)="macos" as sed is only called for darwin anyway. Just disabling the test completely instead of conditionally should simplify the line even more.
There was a problem hiding this comment.
So contrary to the original comment, you're suggesting we use a more fragile approach because it will fail noisily. That's certainly an advantage, at the expense of regular maintenance for a minor transient failure.
The comment about substituteInPlace being more common in nixpkgs is demonstrably false.
|
I'll leave this for others to resolve. I don't particularly mind how it is done. Any color bike shed will do. |
|
I've got a patch for this ready for push, just waiting for #21742 to land |
|
That's good, but it's a shame that such an important package stays broken on |
|
I just pushed a fix in 7435fef. |
|
Please keep in mind that most people are working on nixpkgs in their free time (I am) and that most people don't get paid for doing so (I'm not). The maintainers provided feedback to your pull request in under two hours, and a (hopefully) complete fix was pushed to master in under 24 hours. I hardly find that slow or un-responsive. As for the critique for the preferred method of patching the issue: Feel free to open a discussion. I can hardly stand the Personally I find the second issue the most problematic with the approach: Imagine the function names change. This will lead to The |
|
Your first paragraph goes the other direction, too. To be clear, I think your feedback had definitely valid elements and your experience with Nix means that it's worth paying attention to. What I objected to is the attempt to justify opinion with falsehoods. The point about the fragility of patch files, for instance, is useful. I think that for something that doesn't impact correctness, I'd rather have a robust fix whose unlikely failure either results in a build failure or a useless-but-hamless build step (ie the regex no longer matches, but upstream changes fixed the problem). Likewise, "Is this a place where |
|
It has been brought to my attention that you're a big contributor in the darwin side of things - sorry, I should've looked at your history of contributions before making the statements in my first paragraph. Please consider them to be withdrawn. I might have overreacted a bit on your earlier comment - sory for that. I actually haven't seen the benefits of your approach - your points for robustness and Shall we settle this and move on to opening/merging more PRs and fixing bugs? After all we both want to make nixpkgs a better place. |
rustc-1.14.0 builds are failing on hydra due to test failures of tcp
functionality that do not look like significant failures. This patch
causes these functions to be ignored on darwin.
Motivation for this change
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)