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

nix log should also work if the log didn't provide any output #6051

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Feb 6, 2022

According to git bisect this was introduced by 50a3586

To summarize: while trying to update Hydra to use Nix 2.6, I realized that yath t/queue-runner/notifications.t is failing because it initially builds a derivation (and the builder doesn't write anything to stdout/stderr). After that it's checked if e.g. nix log succeeds.

While this was the case on the Nix 2.4pre rev of Hydra, it isn't the case anymore on Nix 2.6.

While bisecting I found the problem, it seems as if init() wrongly sets archive_read_support_filter_all if raw would be true.

Also added a regression-test that I used for bisecting.

cc @edolstra

@edolstra edolstra added this to the nix-2.7 milestone Feb 8, 2022
Ma27 added a commit to Ma27/nix that referenced this pull request Feb 20, 2022
A few notes:

* The `echo hi` is needed to make sure that a file that can be read by
  `nix log` is properly created (i.e. some output is needed). This is
  known and to be fixed in NixOS#6051.
* We explicitly ignore the floating-CA case here: the `$out` of `input3`
  depends on `$out` of `input2`. This means that there are actually two
  derivations - I assume that this is because at eval time (i.e.
  `nix-instantiate -A`) the hash of `input2` isn't known yet and the
  other .drv is created as soon as `input2` was built. This is another
  issue on its own, so we ignore the case here explicitly.
@Ma27
Copy link
Member Author

Ma27 commented Feb 27, 2022

@edolstra rebased onto latest master to resolve the merge-conflict. Anything else to fix here? Would love to see this bugfix being merged :)

Ma27 added a commit to Ma27/nix that referenced this pull request Feb 27, 2022
A few notes:

* The `echo hi` is needed to make sure that a file that can be read by
  `nix log` is properly created (i.e. some output is needed). This is
  known and to be fixed in NixOS#6051.
* We explicitly ignore the floating-CA case here: the `$out` of `input3`
  depends on `$out` of `input2`. This means that there are actually two
  derivations - I assume that this is because at eval time (i.e.
  `nix-instantiate -A`) the hash of `input2` isn't known yet and the
  other .drv is created as soon as `input2` was built. This is another
  issue on its own, so we ignore the case here explicitly.
Ma27 added a commit to Ma27/nix that referenced this pull request Feb 28, 2022
A few notes:

* The `echo hi` is needed to make sure that a file that can be read by
  `nix log` is properly created (i.e. some output is needed). This is
  known and to be fixed in NixOS#6051.
* We explicitly ignore the floating-CA case here: the `$out` of `input3`
  depends on `$out` of `input2`. This means that there are actually two
  derivations - I assume that this is because at eval time (i.e.
  `nix-instantiate -A`) the hash of `input2` isn't known yet and the
  other .drv is created as soon as `input2` was built. This is another
  issue on its own, so we ignore the case here explicitly.
@edolstra edolstra modified the milestones: nix-2.7, nix-2.8 Mar 3, 2022
this->source = &source;

if (!raw) {
archive_read_support_filter_all(archive);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

factor out?

@Ma27
Copy link
Member Author

Ma27 commented Mar 14, 2022

Rebased onto latest master to resolve the merge conflict.

May I ask if there was any other reason that this didn't land in 2.7 other than the merge-conflict?

factor out?

Personally I'm not a big fan of delaying (trivial) bugfixes with these kinds of requests, especially if the fix is a single revert. But if you want me to, why not, otherwise I'd suggest to do this in a different PR.

@Ma27
Copy link
Member Author

Ma27 commented Mar 15, 2022

ping @edolstra @thufschmitt :)

@Ma27
Copy link
Member Author

Ma27 commented Mar 24, 2022

cc @edolstra @thufschmitt anything missing here? :)

tests/log.sh Outdated Show resolved Hide resolved
This reverts commit 50a3586.

With this change Nix fails to open bzip2 logfiles that were created from
builds with no stdout/stderr.
@Ma27
Copy link
Member Author

Ma27 commented Mar 24, 2022

@edolstra done! (also rebased against master)

@edolstra edolstra merged commit 55bc524 into NixOS:master Mar 25, 2022
@edolstra
Copy link
Member

Thanks!

@Ma27 Ma27 deleted the fix-empty-nix-log branch March 25, 2022 09:47
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.

3 participants