build-remote: Implement in C++ #981
Conversation
|
Ref #341 |
|
Shouldn't this also remove |
|
That needs to be coordinated with a change in NixOS. |
|
@shlevy cool, let's then keep a list of TODOs somewhere for NixOS so this is not forgotten. |
|
cc @edolstra |
| @@ -112,4 +112,14 @@ running, they should use the same <envar>NIX_CURRENT_LOAD</envar> | |||
| file. Maybe in the future <filename>build-remote.pl</filename> will | |||
| look at the actual remote load.</para> | |||
|
|
|||
| <para>Additionally, you may define the environment variable | |||
| <envar>NIX_REMOTE_USE_SUBSTITUTES</envar>, which will instruct | |||
There was a problem hiding this comment.
Why an environment variable instead of a nix.conf option?
There was a problem hiding this comment.
I expected the options in nix.conf to be directly related to Nix, not to the build-remote hook. But I agree that a nix.conf option would be better, if that is acceptable.
|
@edolstra switched to the nix conf option for hook substitutes |
|
|
||
| auto localSystem = argv[1]; | ||
| settings.maxSilentTime = strtoull(argv[2], NULL, 10); | ||
| settings.buildTimeout = strtoull(argv[3], NULL, 10); |
There was a problem hiding this comment.
Calls to strtoull are not checked for parse error.
There was a problem hiding this comment.
It will return ULLONG_MAX, which is a fine default. Note that this is an internal (libexec) tool.
There was a problem hiding this comment.
This prints 0:
unsigned long long res = strtoull("blah", NULL, 10);
printf("%llu\n", res);The function returns ULLONG_MAX in case of overflow.
shlevy
force-pushed
the
build-remote-c++
branch
from
5dd21d1 to
bff3ad7
Compare
|
@edolstra rebased on master. |
|
ping |
|
@shlevy can we fix the tests here? |
|
@edolstra said he was working on a fix |
|
ping |
1 similar comment
|
ping |
|
Ping |
|
Merry Christmas! Ping. |
|
@edolstra Can you give me a time by which I can expect a review, even if that's not in the near future? |
|
|
||
| class machine { | ||
| const std::vector<string> supportedFeatures; | ||
| const std::vector<string> mandatoryFeatures; |
| [&](const string & feature) { | ||
| return std::find(supportedFeatures.begin(), | ||
| supportedFeatures.end(), | ||
| feature) != supportedFeatures.end() || |
There was a problem hiding this comment.
This can be changed to supportedFeatures.count(feature) is supportedFeatures is a set.
| auto machines = std::vector<machine>{}; | ||
| auto confFile = std::ifstream{conf}; | ||
| if (confFile.good()) { | ||
| confFile.exceptions(std::ifstream::badbit); |
There was a problem hiding this comment.
C++ streams considered harmful, among other things due to the wacky error/exception handling. I would just do tokenizeString(readFile(conf), "\n").
|
@edolstra what's the plan for this PR? |
|
🍾 |
|
Was this the last remnant of perl or is there other stuff left? I can't wait to see perl disappear from release.nix 😄 |
|
Thanks @shlevy ! |
|
It appears that |
|
Commit using futimes pushed. |
|
@edolstra now we need NixOS/nixpkgs#17697 to fix tests, does that mean we finally get #1134 ? 🙈 |
No description provided.