-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
treewide: enable working, fix fixable, disable broken tests #44825
treewide: enable working, fix fixable, disable broken tests #44825
Conversation
# sed -i -e 's@dbus/[^ ]*\.\(test\|vala\)@@g' tests/Makefile.am | ||
# ''; | ||
# | ||
# checkInputs = [ automake_1_15 ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like we should just leave off it isn't working.
postPatch = '' | ||
for a in test/Makefile.in test/format_test/format_checks.sh.in ; do | ||
substituteInPlace $a \ | ||
--replace /bin/bash ${bash}/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's considered more portable to do stdenv.shell
instad of ${bash}/bin/bash
.
@@ -26,9 +25,7 @@ in stdenv.mkDerivation rec { | |||
|
|||
propagatedBuildInputs = [ zlib ]; | |||
|
|||
# it's hard to cross-run tests and some check programs didn't compile anyway | |||
makeFlags = stdenv.lib.optional (!doCheck) "check_PROGRAMS="; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might still be needed when stdenv.hostPlatform /= stdenv.buildPlatform;
@@ -22,14 +22,11 @@ stdenv.mkDerivation rec { | |||
pango gcr gdk_pixbuf atk p11-kit | |||
]; | |||
|
|||
# In 3.20.1, tests do not support Python 3 | |||
checkInputs = [ dbus python2 ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you keep the comment?
pkgs/tools/misc/parted/default.nix
Outdated
|
||
# The `t0400-loop-clobber-infloop.sh' test wants `mkswap'. | ||
preCheck = '' | ||
export PATH=${utillinux}/sbin:$PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should no longer be needed, sbin is a symlink to bin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from a few things.
a3dc51c
to
89efaf6
Compare
I rebased the fixes to comments and added all the required patches to build all the packages this touches with `doCheckByDefault` enabled (unless they conflicted, those I will respin later). I also built all of said packages.
So, in short, I think this should be ready to merge, but even if you see no issues, please wait till noon UTC tomorrow, just in case.
@matthewbauer
This looks like we should just leave off it isn't working.
Yep, removed.
I think it's considered more portable to do ```stdenv.shell``` instad of ```${bash}/bin/bash.
Done.
This might still be needed when stdenv.hostPlatform /= stdenv.buildPlatform;
Nope, before this `doCheck` was set to `true` so that code was never ever run.
@jtojnar
Could you keep the comment?
Done.
The original version is kept as `docheck/continues-in-a-big-way-v1` branch in my repo for `diff`ing.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Make sure you follow the Hydra staging jobset after this merges. Occasionally new tests will cause headaches on the Hydra infrastructure.
In "Motivation for this change", can you please describe the actual motivation rather than point to different PRs/issues? For example, this PR's motivation refers to #39464, which helpfully is entitled "stdenv: implement most of #33599" and whose motivation says "see #39463". That's not very helpful with a PR that causes an enormous amount of code churn (127 files changed). |
Since it was used before, let's keep using it.
259f9c1
to
35c9435
Compare
I looked over this with a fresh head and could find only several minor OCD things.
Old versions are kept as `docheck/continues-in-a-big-way-v2` (the version before today) and `docheck/continues-in-a-big-way-v2-ocd` (the version with ocd spelled out) for diffing. I squashed OCD patches into the current version.
It merges with staging, it evaluates, everything compiles. LGTM. It's perfect. Let's merge.
|
ping @matthewbauer @Ericson2314 @7c6f434c
Could somebody please merge this? I think this can only get worse, not better, with coming conflicts.
|
Seems fine, and with all the semi-auto-updates we need the tests eventually enabled… |
Motivation for this change
#33599 ("Run check phase by default"). This is yet another^4 (v2) followup to #39464 ("stdenv: implement most of #33599").
Things done
doCheckByDefault
set, so I think it should work sane enough on top of Nixpkgs too.Expectations