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

bash_5 fails on hydra but succeeds locally #115177

Closed
meithecatte opened this issue Mar 5, 2021 · 6 comments
Closed

bash_5 fails on hydra but succeeds locally #115177

meithecatte opened this issue Mar 5, 2021 · 6 comments
Assignees

Comments

@meithecatte
Copy link
Contributor

Describe the bug
The most recent Hydra build of bash_5 failed with

configure: WARNING: unrecognized options: --disable-static
building
build flags: -j2 -l2 SHELL=/nix/store/f7jzmxq9bpbxsg69cszx56mw14n115n5-bash-4.4-p23/bin/bash
cd . && autoconf
/nix/store/f7jzmxq9bpbxsg69cszx56mw14n115n5-bash-4.4-p23/bin/bash: autoconf: command not found
make: *** [Makefile:785: configure] Error 127

However, running nix-build -A bash_5 succeeds and produces the store path /nix/store/yqk7pzd398gxnd7ph6vbka7cm1hvb6ai-bash-5.1-p4 - exactly as listed on the Hydra page.

Expected behavior
The results should agree.

Notify maintainers
The package is maintained by @peti @dtzWill but I suspect there's something deeper at play here.

Metadata

  • system: "x86_64-linux"
  • host os: Linux 5.4.99, NixOS, 21.05pre272380.f6b5bfdb470 (Okapi)
  • multi-user?: yes
  • sandbox: yes
  • version: nix-env (Nix) 2.3.10
  • channels(kuba): "nixpkgs-21.05pre272380.f6b5bfdb470"
  • channels(root): "nixos-21.05pre272380.f6b5bfdb470"
  • nixpkgs: /home/kuba/.nix-defexpr/channels/nixpkgs

Maintainer information:

# a list of nixpkgs attributes affected by the problem
attribute: bash_5 bashInteractive_5
# a list of nixos modules affected by the problem
module:
@andersk
Copy link
Contributor

andersk commented Mar 20, 2021

This is a nondeterministic failure. I just saw it happen locally, but I reran the same build and it succeeded. I think I figured out why—note the following difference between the bash log and the bash_5 log.

bash:

applying patch /nix/store/d6a4ngpmijv1sk5q7al7ka2lhy8m5ryc-pgrp-pipe-4.4.patch
patching file ./configure
patching file ./configure.ac

bash_5:

applying patch /nix/store/hr5f8216rr5dzbl4ib4gzl76yp4bbrip-pgrp-pipe-5.1.patch
patching file ./configure
Hunk #1 succeeded at 20407 with fuzz 2 (offset 4095 lines).
Not setting time of file ./configure (time mismatch)
patching file ./configure.ac
Hunk #1 succeeded at 1159 with fuzz 2 (offset 51 lines).
Not setting time of file ./configure.ac (time mismatch)

The 4.4 patch applies cleanly with matched timestamps, so configure and configure.ac get their timestamps set to the ones in the patch file, with configure getting a significantly newer timestamp than configure.ac. But the 5.1 patch applies with mismatched timestamps, so configure and configure.ac both get their timestamps set to the current time, in that order. It’s then up to rounding whether this rule in Makefile.in gets triggered:

# comment out for distribution
$(srcdir)/configure:	$(srcdir)/configure.ac $(srcdir)/aclocal.m4 $(srcdir)/config.h.in
	cd $(srcdir) && autoconf

This patch was added in #77196 (cc @raboof @FRidh).

@meithecatte
Copy link
Contributor Author

If we're patching configure.ac, perhaps configure should get generated by autoconf and not patched in?

@vcunat
Copy link
Member

vcunat commented Mar 20, 2021

Well, I suspect it might complicate bootstrapping if we add those dependencies here. Note that the patch fixes both of the files equally, so it should be just a timestamp issue. 🤔 perhaps just swap the two parts inside each patch, so that configure is patched after configure.ac?

@raboof
Copy link
Member

raboof commented Mar 21, 2021

the 5.1 patch applies with mismatched timestamps, so configure and configure.ac both get their timestamps set to the current time, in that order. It’s then up to rounding whether this rule in Makefile.in gets triggered

Ouch, great find!

@raboof raboof self-assigned this Mar 21, 2021
@raboof
Copy link
Member

raboof commented Mar 21, 2021

It would be cool if we could tell patch to fail rather than warn when the timestamps don't match, but it looks like that isn't possible right now (https://savannah.gnu.org/bugs/index.php?60266)

raboof added a commit to raboof/nixpkgs that referenced this issue Mar 21, 2021
When, after patching, `configure.ac` is newer than `configure`, the
Makefile will try to regenerate `configure` from `configure.ac`.

While that might usually be desirable, in this case we want to keep
bootstrapping simple and directly use the `configure` from the package
so we can avoid a dependency on automake.

Previously, we used the `-T` parameter to automake to make sure the
timestamps were okay. However, this is brittle when we update: when the
timestamp of the original file changes, and no longer matches the
timestamp of the original file in the patch, `patch` will show a warning
but otherwise continue without updating the timestamp.

This PR changes things so we only patch `configure`, so that will always
have a newer timestamp.

Refs NixOS#115177
raboof added a commit to raboof/nixpkgs that referenced this issue Mar 22, 2021
When, after patching, `configure.ac` is newer than `configure`, the
Makefile will try to regenerate `configure` from `configure.ac`.

While that might usually be desirable, in this case we want to keep
bootstrapping simple and directly use the `configure` from the package
so we can avoid a dependency on automake.

Previously, we used the `-T` parameter to automake to make sure the
timestamps were okay. However, this is brittle when we update: when the
timestamp of the original file changes, and no longer matches the
timestamp of the original file in the patch, `patch` will show a warning
but otherwise continue without updating the timestamp.

This PR changes things so we only patch `configure`, so that will always
have a newer timestamp.

We will update bash-4.4 in a separate PR (but that one has a bigger rebuild
impact so will have to target staging)

Refs NixOS#115177
@vcunat
Copy link
Member

vcunat commented Mar 27, 2021

Fixes merged.

@vcunat vcunat closed this as completed Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants