Stdenv setup test by happysalada · Pull Request #132490 · NixOS/nixpkgs

happysalada · 2021-08-03T03:52:20Z

Motivation for this change

@siraben @vcunat
Here is the PR where I've grouped all the changes that I think should pass and would be fairly uncontroversial.
I hope I haven't missed anything.
I didn't know if I should pick master or staging as a base. It seemed intuitive to me to pick master to lower the chance of unrelated failures, let me know if you want me to base it on staging.

The following PRs are included

I'm putting this in a draft stage until someone has a chance to test and make sure it doesn't break anything.

Things done

(cherry picked from commit bf40c3054db4c552a4f229738d84f11755e9d623)

(cherry picked from commit 32ee9a7)

(cherry picked from commit 213ea52)

(cherry picked from commit f5066a0)

(cherry picked from commit c1b7aa9)

(cherry picked from commit 66d8c2a)

(cherry picked from commit b2490c97b3ba555ca4f76c48d3c8904c1a4775f4)

happysalada · 2021-08-03T12:40:44Z

I have just checked the failure and it seems to be related to aws-c-common

2021-08-03T07:56:40.6352244Z 380/380 Test #380: promise_test_multiple_waiters ........................................***Timeout 1500.06 sec
2021-08-03T07:56:40.6353313Z
2021-08-03T07:56:40.6353741Z
2021-08-03T07:56:40.6354335Z 99% tests passed, 1 tests failed out of 380
2021-08-03T07:56:40.6354831Z
2021-08-03T07:56:40.6355402Z Total Test time (real) = 1527.04 sec
2021-08-03T07:56:40.6356095Z
2021-08-03T07:56:40.6356962Z The following tests FAILED:
2021-08-03T07:56:40.6358411Z         380 - promise_test_multiple_waiters (Timeout)
2021-08-03T07:56:40.6359179Z Errors while running CTest
2021-08-03T07:56:40.6359923Z make: *** [Makefile:126: test] Error 8
2021-08-03T07:56:40.6777286Z builder for '/nix/store/m002v7maav5m82hsq1dg8cq4p6s24fqq-aws-c-common-0.6.8.drv' failed with exit code 2

I wonder if there is a change that this test is flaky.
I'm going to try evaluation again and test on local.

@GrahamcOfBorg eval

Mindavi · 2021-08-03T20:44:23Z

I've seen aws-c-common being flaky too in the last days, or you might've just gotten unlucky. Not sure which it is.

It seems to have solved itself on my end though.

happysalada · 2021-08-04T02:18:05Z

@Mindavi thank you for the comment.
I tried removing one commit that included named ref, and the test pass now.
I'll do more experimentation to see if I was just unlucky. I have some other PR that have the same failures, I'll try to force push to trigger the evaluation again.

Hydra: ?compare=1692843

vcunat · 2021-08-07T09:33:21Z

Jobset set up at https://hydra.nixos.org/jobset/nixpkgs/pr-132490 (just x86_64-linux now, low priority) EDIT: direct comparison link https://hydra.nixos.org/eval/1693558?compare=1692843

I'll try to have a closer look at the code, too.

happysalada · 2021-08-07T12:18:25Z

Thank you so much for this!

happysalada · 2021-08-11T05:22:40Z

It seems there are 20 new failures but 12 newly passing ones
https://hydra.nixos.org/eval/1693558?compare=1692843#tabs-now-fail

Some of the failures look they are due to time out.
From some of the hard failures, I've looked at the logs of a few but couldn't find anything linked to this PR.
https://hydra.nixos.org/build/149685699/nixlog/2
https://hydra.nixos.org/build/149685620/nixlog/2
https://hydra.nixos.org/build/149709576/nixlog/2
https://hydra.nixos.org/build/149701857/nixlog/2
https://hydra.nixos.org/build/149681561/nixlog/2

@vcunat I'm a little new to this, am I missing something or are those PRs fine to merge ?

vcunat · 2021-08-18T09:23:40Z

I also locally checked cross-building from x86_64-linux (various packages and targets).

Some commits apparently haven't been reviewed by anyone except the author (not even myself so far), so that's the only missing part now, I think. At least some quick review.

happysalada · 2021-08-18T10:19:22Z

@vcunat If you mean that those commits need more reviews, understood.

vcunat · 2021-08-18T10:49:50Z

I would certainly prefer that. If noone steps up, I'd hope to have a look during this weekend.

7c6f434c · 2021-08-18T11:24:43Z

pkgs/build-support/setup-hooks/move-docs.sh

    forceShare=${forceShare:=man doc info}
-    if [ -z "$forceShare" -o -z "$out" ]; then return; fi
+    if [[ -z "$forceShare" || -z "$out" ]]; then return; fi


Wait, what exactly the first line here is supposed to do? It seems to check if forceShare is empty, and if so assign man doc info to it. Twice (once via default value assignment :=, second via the actual assignment). In any case, can forceShare ever be empty afterwards?

You're right here, I've tested everything I could think of, and there is no way (that I could think of) that forceShare could be empty. Let me simplify that condition (I'll do it in the original PR).

7c6f434c · 2021-08-18T11:28:46Z

pkgs/stdenv/generic/setup.sh

-    if [ -n "${showBuildStats:-}" ]; then
-        times > "$NIX_BUILD_TOP/.times"
-        local -a times=($(cat "$NIX_BUILD_TOP/.times"))
+    if [[ -n "${showBuildStats:-}" ]]; then


Is there a reason for this line change? Sounds like [ is enough here…

This was more around trying to have a single way to do conditionals.
As I understand it, you never loose by using [[ instead of [ and there are less pitfalls.
I do understand that it might be controversial. If you prefer [, I'm happy to try to avoid that change where unecessary in future PRs.

I doubt there is a coherent style guidance on that… this change was just a part of a kind of unrelated (except for adjacency) commit.

(I myself probably lean towards «POSIX except where extension make things much simpler», but I do not change stdenv much)

7c6f434c · 2021-08-18T11:29:23Z

pkgs/stdenv/generic/setup.sh

-        local -a times=($(cat "$NIX_BUILD_TOP/.times"))
+    if [[ -n "${showBuildStats:-}" ]]; then
+        local -a buildTimesArray
+        IFS=" " read -r -a buildTimesArray < "$NIX_BUILD_TOP/.times"


Hmm, have you just removed the line that created the .times file?

I did make a mistake here.
I was originally thinking about combining those steps to not create the file anymore. (I wanted to put the the times command directly as an input). Now that i think about it though, it might not be good to get rid of that file.
My original thought was that since it's echoed, perhaps it's not needed to keep a record.
I'll add it back just in case it is used actually.

I have no strong opinion on keeping the file, but in case of debugging functionality I would indeed err on the side of giving more ways to find data.

7c6f434c · 2021-08-18T11:38:47Z

(I have read the changes till the end)

happysalada · 2021-08-18T15:44:00Z

@7c6f434c If this gets accepted to merge, I will not include the change around the .times at all.
I think it's going through a file because the times command is two lines. A separator just on white space doesn't work so well anymore. I'm going to just close that PR altogether.

happysalada · 2021-08-30T01:23:25Z

This is probably all the reviews this was going to get. I'm going to go ahead and merge those PRs to staging for now.

happysalada · 2021-08-30T01:37:52Z

This PR was just for testing, so closing this.

happysalada added 7 commits August 3, 2021 12:42

stdenv: arithmetic fixes

52635cc

(cherry picked from commit bf40c3054db4c552a4f229738d84f11755e9d623)

stdenv: remove combined command conditional

795b695

(cherry picked from commit 32ee9a7)

stdenv: reduce io

c6a0e9c

(cherry picked from commit 213ea52)

stdenv: declare missing variables

006a5db

(cherry picked from commit f5066a0)

stdenv: clarify array semantics

fff3215

(cherry picked from commit c1b7aa9)

stdenv: remove bash version compatibility hack

97cf5e5

(cherry picked from commit 66d8c2a)

move-docs.sh: shellcheck 2166

2ae1bd8

(cherry picked from commit b2490c97b3ba555ca4f76c48d3c8904c1a4775f4)

happysalada requested review from Ericson2314 and matthewbauer as code owners August 3, 2021 03:52

github-actions bot added the 6.topic: stdenv Standard environment label Aug 3, 2021

happysalada marked this pull request as draft August 3, 2021 03:52

happysalada changed the base branch from master to staging August 3, 2021 04:01

happysalada changed the base branch from staging to master August 3, 2021 04:01

happysalada force-pushed the stdenv_setup_test branch from f684c60 to 2ae1bd8 Compare August 3, 2021 12:42

Merge recent branch 'master' into PR NixOS#132490

95b409e

Hydra: ?compare=1692843

happysalada mentioned this pull request Aug 18, 2021

stdenv: arithmetic fixes #130597

Merged

11 tasks

happysalada mentioned this pull request Aug 18, 2021

[RFC 0099] Change default shell from bash to oil NixOS/rfcs#99

Closed

7c6f434c reviewed Aug 18, 2021

View reviewed changes

happysalada mentioned this pull request Aug 18, 2021

stdenv: clarify array semantics #130598

Closed

11 tasks

happysalada closed this Aug 30, 2021

happysalada deleted the stdenv_setup_test branch April 28, 2023 20:00

Uh oh!

Conversation

happysalada commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Things done

Uh oh!

happysalada commented Aug 3, 2021

Uh oh!

Mindavi commented Aug 3, 2021

Uh oh!

happysalada commented Aug 4, 2021

Uh oh!

vcunat commented Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

happysalada commented Aug 7, 2021

Uh oh!

happysalada commented Aug 11, 2021

Uh oh!

vcunat commented Aug 18, 2021

Uh oh!

happysalada commented Aug 18, 2021

Uh oh!

vcunat commented Aug 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

7c6f434c commented Aug 18, 2021

Uh oh!

happysalada commented Aug 18, 2021

Uh oh!

happysalada commented Aug 30, 2021

Uh oh!

happysalada commented Aug 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

happysalada commented Aug 3, 2021 •

edited

Loading

vcunat commented Aug 7, 2021 •

edited

Loading