Skip to content
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

Fix log-prefix of nix build -L #3661

Closed
wants to merge 1 commit into from
Closed

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Jun 4, 2020

While playing around with Nix master, I realized that the log-prefix is
broken when building a derivation on a remote builder:

~/nixpkgs → ../nix/inst/bin/nix build --experimental-features 'nix-command flakes'  -L -f . hello
lder0> unpacking sources
lder0> unpacking source archive /nix/store/3x7dwzq014bblazs7kq20p9hyzz0qh8g-hello-2.10.tar.gz
lder0> source root is hello-2.10
lder0> setting SOURCE_DATE_EPOCH to timestamp 1416139241 of file hello-2.10/ChangeLog
lder0> patching sources
lder0> configuring
lder0> configure flags: --disable-dependency-tracking --prefix=/nix/store/55xlqch5wdc3vbkjl3v47bn5if63n65c-hello-2.10
[1/0/1 built]
error: interrupted by the user

It seems as the five chars of the string hello have been replaced by
the builder's name (ssh://builder). I suspect that this is related to
fmt taking args as reference[1].

I'm not sure why, but this can be avoided by not appending another
formatted string to i->s. I worked around this by modifying i->s
only once.

I can reproduce this on a recent master[2] and on a recent flakes[3]. In
the latter case, e.g. nix flake check -L is also affected.

[1] 334b8f8
[2] bfa1acd
[3] 81cafda

While playing around with Nix master, I realized that the log-prefix is
broken when building a derivation on a remote builder:

```
~/nixpkgs → ../nix/inst/bin/nix build --experimental-features 'nix-command flakes'  -L -f . hello
lder0> unpacking sources
lder0> unpacking source archive /nix/store/3x7dwzq014bblazs7kq20p9hyzz0qh8g-hello-2.10.tar.gz
lder0> source root is hello-2.10
lder0> setting SOURCE_DATE_EPOCH to timestamp 1416139241 of file hello-2.10/ChangeLog
lder0> patching sources
lder0> configuring
lder0> configure flags: --disable-dependency-tracking --prefix=/nix/store/55xlqch5wdc3vbkjl3v47bn5if63n65c-hello-2.10
[1/0/1 built]
error: interrupted by the user
```

It seems as the five chars of the string `hello` have been replaced by
the builder's name (ssh://builder). I suspect that this is related to
`fmt` taking `args` as reference[1].

I'm not sure why, but this can be avoided by not appending another
formatted string to `i->s`. I worked around this by modifying `i->s`
only once.

I can reproduce this on a recent `master`[2] and on a recent `flakes`[3]. In
the latter case, e.g. `nix flake check -L` is also affected.

[1] 334b8f8
[2] bfa1acd
[3] 81cafda
edolstra added a commit that referenced this pull request Jun 5, 2020
Alternative fix to #3661. The cause was that 'name' is a
std::string_view into a temporary which could get overwritten.
@edolstra
Copy link
Member

edolstra commented Jun 5, 2020

Thanks, I've fixed this in a different way in 39e84c3.

@edolstra edolstra closed this Jun 5, 2020
@Ma27
Copy link
Member Author

Ma27 commented Jun 5, 2020

Thanks, missed that. Your fix is actually way better :)

@Ma27 Ma27 deleted the build-progress branch June 5, 2020 09:24
@Ma27 Ma27 restored the build-progress branch June 5, 2020 09:24
@Ma27 Ma27 deleted the build-progress branch June 5, 2020 09:24
@edolstra
Copy link
Member

edolstra commented Jun 5, 2020

Yeah, it was a lesson for me that changing the return type of a function from std::string to std::string_view is a bad idea. C++ is not Rust...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants