Tell in detail why remote building fails on -v #3927

nh2 · 2020-08-13T04:34:28Z

Motivation

See #1572. Further improves upon #3897 (CC @bburdette).

Also further improve the final error message, giving a practical example of
how to set machine features because that's what most people need to succeed
in building large packages remotely.

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

src/build-remote/build-remote.cc

src/libstore/build.cc

bburdette · 2020-08-18T18:58:24Z

Got a screenshot of output from this?

src/build-remote/build-remote.cc

nh2 · 2020-09-04T13:44:35Z

I've squashed the commit suggestions.

nh2 · 2020-09-04T13:47:11Z

Got a screenshot of output from this?

@bburdette I didn't make a screenshot but it's litearlly just another of the many lines in the -v output, like

...
declined remote machine 'static-haskell-nix-ci' for building '/nix/store/package-hash1234' because the derivation does not have all mandatory features to build on that machine: [ big-parallel ]
...

domenkozar · 2020-11-03T10:53:31Z

@nh2 could you rebase and we'll merge it?

nh2 · 2020-11-25T19:49:01Z

@Ericson2314 Can you help me rebase it?

You deleted the message I enhanced in https://github.com/NixOS/nix/pull/3927/files#diff-19f999107b609d37cfb22c58e7f0bc1cf76edf1180e238dd6389e03cc279b604R4885 as part of #4114 here:

3bab1c5#diff-5784d1ad7c22601c75b86872534049bea5173377de5d61b7446452beabc16d3fL5324

Thanks!

nh2 · 2021-06-02T03:13:38Z

@Ericson2314 short ping :)

domenkozar · 2021-06-02T15:23:32Z

🙏 this one is a huge UX improvement, it would be a shame to let it stale.

stale · 2022-04-16T01:26:42Z

I marked this as stale due to inactivity. → More info

nh2 · 2022-05-27T12:58:29Z

@Ericson2314 Short ping 2 years later ^ :)

bitonic · 2022-05-27T13:01:58Z

Just to chime in, I lost half an hour on this today. It'd be great to get this merged.

Ericson2314 · 2023-03-11T16:21:00Z

Ah! I am seeing and all the pings this now :(

Ericson2314 · 2023-03-11T16:29:36Z

As penance, I will just rebase this myself.

Ericson2314 · 2023-03-11T21:45:43Z

@nh2 I have rebased, and applied the PR description template.

Ericson2314 · 2023-03-11T21:48:05Z

As to the content of the new error messages, I hope @fricklerhandwerk can review them.

Also further improve the final error message, giving a practical example of how to set machine features because that's what most people need to succeed in building large packages remotely.

fricklerhandwerk

I think we should be more explicit with the expectations that were failed as represented by the error message. I'd be in favor of having a very structured presentation similar to what we had in the profile conflict PR, e.g

error condition

  expected: ...

  actual: ...

hint on what to do

@roberth we may want to write a guideline :)

fricklerhandwerk · 2023-03-12T08:39:02Z

src/build-remote/build-remote.cc

+                        debug("declined remote machine '%s' for building '%s' because it does not have the needed system type '%s", m.storeUri, drvstr, neededSystem);
+                    } else if (!m.allSupported(requiredFeatures)) {
+                        std::string joinedFeatures = concatStringsSep(" ", requiredFeatures); // includes leading space
+                        debug("declined remote machine '%s' for building '%s' because it does not have all required features: [ %s ]", m.storeUri, drvstr, joinedFeatures);


This should be explicit on what was expected and what is available.

fricklerhandwerk · 2023-03-12T08:40:04Z

src/build-remote/build-remote.cc

+                        debug("declined remote machine '%s' for building '%s' because it does not have all required features: [ %s ]", m.storeUri, drvstr, joinedFeatures);
+                    } else if (!m.mandatoryMet(requiredFeatures)) {
+                        std::string joinedFeatures = concatStringsSep(" ", requiredFeatures); // includes leading space
+                        debug("declined remote machine '%s' for building '%s' because the derivation does not have all mandatory features to build on that machine: [ %s ]", m.storeUri, drvstr, joinedFeatures);


I don't understand what this means. I thought a machine may be lacking features to build a drv, how would it work the other way round?

~~likely the message is backwards~~

It's accurate as is, but "decline" might not be the best word perhaps (for all of these)

fricklerhandwerk · 2023-03-12T08:45:58Z

src/libstore/build/worker.cc

+                            "For example, to enable the 'big-parallel' feature, use:"
+                            "\n    --builders 'yourbuilderhostname - - - - big-parallel'"
+                            "\nhttps://nixos.org/nix/manual/stable/advanced-topics/distributed-builds"


This should instead refer to the features section in man nix.conf as we did for other hints in error messages recently.

fricklerhandwerk · 2023-03-12T08:46:49Z

src/libstore/build/worker.cc

+                            "For example, to enable the 'big-parallel' feature, use:"
+                            "\n    --builders 'yourbuilderhostname - - - - big-parallel'"
+                            "\nhttps://nixos.org/nix/manual/stable/advanced-topics/distributed-builds"
+                            "\nYou can enable -v verbosity to see why any machine "


Suggested change

"\nYou can enable -v verbosity to see why any machine "

"\nEnable -v verbosity to show why any machine "

fricklerhandwerk · 2023-03-12T08:47:32Z

src/build-remote/build-remote.cc

+                    } else if (neededSystem != "builtin" &&
+                        std::find(m.systemTypes.begin(), m.systemTypes.end(), neededSystem)
+                        == m.systemTypes.end()) {
+                        debug("declined remote machine '%s' for building '%s' because it does not have the needed system type '%s", m.storeUri, drvstr, neededSystem);


Which one does it have then?

roberth · 2023-03-12T18:25:47Z

I think we should be more explicit with the expectations that were failed as represented by the error message. I'd be in favor of having a very structured presentation similar to what we had in the profile conflict PR, e.g
error condition

  expected: ...

  actual: ...

hint on what to do

Explaining why nothing doesn't satisfy a set of constraints is always a recipe for long and complicated error messages. That's just the nature of the problem: there's a lot of data involved and we can't guess what should match up with what.
In this case we'd have to track exactly why each of the remote builders does not satisfy the requirements and pass that information to the exception constructor.

Realistically the error message would have to be

error condition

expected:

not builder 1
  reasons

not builder 2
  reasons

not builder 3
  reasons

...

You could sort the build machines by relevance and truncate after some limit (say, 3) to reduce the length of the message.

To make sure that the algorithm for checking and the algorithm for giving reasons are in sync, we'll want to use some abstractions that we don't have yet, or perhaps we should accept that we're computing and remembering too much in the happy flow.

fricklerhandwerk · 2023-03-12T23:04:55Z

If I read the code correctly, the debug outputs are per machine, so all we need at this point are slightly more verbose texts that simply also contain the expected properties. And I only propose to make them multiline and possibly indented to make sure that the additional visual noise doesn't preclude noticing the relevant bits.

I don't see why this would require additional infrastructure.

roberth · 2023-03-13T10:22:52Z

I assumed that you wanted to incorporate the details into the exception message instead, so that extra log verbosity isn't needed. If it remains a lvlDebug thing, the performance cost isn't too important.

fricklerhandwerk · 2023-03-13T15:18:28Z

@roberth I have no opinion on whether it should be in the exception. Setting up the infra to lift it to the exception level sounds like quite some work, so I'd rather merge it as debug messages and allow for following up in a more principled manner when time allows.

Ericson2314 · 2023-04-03T03:14:24Z

#7865 structured exceptions would help with this.

roberth · 2023-09-28T01:17:49Z

src/build-remote/build-remote.cc

-                        m.mandatoryMet(requiredFeatures))
-                    {
+                    if (!m.enabled) {
+                        debug("declined remote machine '%s' because it is disabled", storeUri);


Suggested change

debug("declined remote machine '%s' because it is disabled", storeUri);

debug("avoiding remote machine '%s' because it is disabled", storeUri);

How about avoid for all of these?

The problem with decline is that it seems like the machine is initiating the remote build or something.

This was referenced Aug 13, 2020

error messages for issue 2238 #3897

Merged

nix no longer tells me why I can't build remotely #1572

Open

Ericson2314 reviewed Aug 14, 2020

View reviewed changes

src/build-remote/build-remote.cc Outdated Show resolved Hide resolved

nh2 force-pushed the issue-1572-log-remote-builder-decline-reasons branch 2 times, most recently from 7468a80 to 5063f8c Compare August 18, 2020 00:58

edolstra reviewed Aug 18, 2020

View reviewed changes

src/build-remote/build-remote.cc Outdated Show resolved Hide resolved

edolstra reviewed Aug 18, 2020

View reviewed changes

src/libstore/build.cc Outdated Show resolved Hide resolved

edolstra reviewed Sep 4, 2020

View reviewed changes

src/build-remote/build-remote.cc Outdated Show resolved Hide resolved

nh2 force-pushed the issue-1572-log-remote-builder-decline-reasons branch from 581e999 to ddd4556 Compare September 4, 2020 13:43

domenkozar added the error-messages Confusing messages and better diagnostics label Jul 28, 2021

stale bot added the stale label Apr 16, 2022

stale bot removed the stale label May 27, 2022

fricklerhandwerk added the UX The way in which users interact with Nix. Higher level than UI. label Sep 9, 2022

Ericson2314 self-assigned this Mar 11, 2023

Ericson2314 force-pushed the issue-1572-log-remote-builder-decline-reasons branch from ddd4556 to 28a52ff Compare March 11, 2023 21:44

Ericson2314 requested a review from thufschmitt as a code owner March 11, 2023 21:44

Ericson2314 requested a review from fricklerhandwerk March 11, 2023 21:46

Tell in detail why remote building fails on -v. See NixOS#1572

b1d22a4

Also further improve the final error message, giving a practical example of how to set machine features because that's what most people need to succeed in building large packages remotely.

Ericson2314 force-pushed the issue-1572-log-remote-builder-decline-reasons branch from 28a52ff to b1d22a4 Compare March 11, 2023 21:50

fricklerhandwerk requested changes Mar 12, 2023

View reviewed changes

roberth reviewed Sep 28, 2023

View reviewed changes

9999years mentioned this pull request Dec 11, 2023

Print the value in error: cannot coerce messages #9553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tell in detail why remote building fails on -v #3927

Tell in detail why remote building fails on -v #3927

nh2 commented Aug 13, 2020 •

edited by Ericson2314

Loading

bburdette commented Aug 18, 2020

nh2 commented Sep 4, 2020

nh2 commented Sep 4, 2020

domenkozar commented Nov 3, 2020

nh2 commented Nov 25, 2020

nh2 commented Jun 2, 2021

domenkozar commented Jun 2, 2021

stale bot commented Apr 16, 2022

nh2 commented May 27, 2022

bitonic commented May 27, 2022

Ericson2314 commented Mar 11, 2023 •

edited

Loading

Ericson2314 commented Mar 11, 2023

Ericson2314 commented Mar 11, 2023

Ericson2314 commented Mar 11, 2023

fricklerhandwerk left a comment

fricklerhandwerk Mar 12, 2023

fricklerhandwerk Mar 12, 2023

tomberek Sep 28, 2023 •

edited

Loading

roberth Sep 28, 2023

fricklerhandwerk Mar 12, 2023

fricklerhandwerk Mar 12, 2023

fricklerhandwerk Mar 12, 2023

roberth commented Mar 12, 2023

fricklerhandwerk commented Mar 12, 2023 •

edited

Loading

roberth commented Mar 13, 2023

fricklerhandwerk commented Mar 13, 2023 •

edited

Loading

Ericson2314 commented Apr 3, 2023

roberth Sep 28, 2023

	"\nYou can enable -v verbosity to see why any machine "
	"\nEnable -v verbosity to show why any machine "

	debug("declined remote machine '%s' because it is disabled", storeUri);
	debug("avoiding remote machine '%s' because it is disabled", storeUri);

Tell in detail why remote building fails on -v #3927

Are you sure you want to change the base?

Tell in detail why remote building fails on -v #3927

Conversation

nh2 commented Aug 13, 2020 • edited by Ericson2314 Loading

Motivation

Context

Checklist for maintainers

Priorities

bburdette commented Aug 18, 2020

nh2 commented Sep 4, 2020

nh2 commented Sep 4, 2020

domenkozar commented Nov 3, 2020

nh2 commented Nov 25, 2020

nh2 commented Jun 2, 2021

domenkozar commented Jun 2, 2021

stale bot commented Apr 16, 2022

nh2 commented May 27, 2022

bitonic commented May 27, 2022

Ericson2314 commented Mar 11, 2023 • edited Loading

Ericson2314 commented Mar 11, 2023

Ericson2314 commented Mar 11, 2023

Ericson2314 commented Mar 11, 2023

fricklerhandwerk left a comment

Choose a reason for hiding this comment

fricklerhandwerk Mar 12, 2023

Choose a reason for hiding this comment

fricklerhandwerk Mar 12, 2023

Choose a reason for hiding this comment

tomberek Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

roberth Sep 28, 2023

Choose a reason for hiding this comment

fricklerhandwerk Mar 12, 2023

Choose a reason for hiding this comment

fricklerhandwerk Mar 12, 2023

Choose a reason for hiding this comment

fricklerhandwerk Mar 12, 2023

Choose a reason for hiding this comment

roberth commented Mar 12, 2023

fricklerhandwerk commented Mar 12, 2023 • edited Loading

roberth commented Mar 13, 2023

fricklerhandwerk commented Mar 13, 2023 • edited Loading

Ericson2314 commented Apr 3, 2023

roberth Sep 28, 2023

Choose a reason for hiding this comment

nh2 commented Aug 13, 2020 •

edited by Ericson2314

Loading

Ericson2314 commented Mar 11, 2023 •

edited

Loading

tomberek Sep 28, 2023 •

edited

Loading

fricklerhandwerk commented Mar 12, 2023 •

edited

Loading

fricklerhandwerk commented Mar 13, 2023 •

edited

Loading