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

fish: 3.0.2 -> 3.1.0 #79941

Merged
merged 1 commit into from Feb 18, 2020
Merged

fish: 3.0.2 -> 3.1.0 #79941

merged 1 commit into from Feb 18, 2020

Conversation

@cole-h
Copy link
Member

@cole-h cole-h commented Feb 12, 2020

Motivation for this change

fish-shell 3.1.0 was just released today with many niceties (including
the ability to have bash-like temporary env vars e.g. VAR="var1" command instead of needing to use env VAR="var1" command). To see the
full list of changes, please visit
https://github.com/fish-shell/fish-shell/releases/tag/3.1.0.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc: @ocharles


Closes #79934.

Copy link
Member

@ocharles ocharles left a comment

LGTM!

@mmahut
Copy link
Member

@mmahut mmahut commented Feb 13, 2020

@GrahamcOfBorg build fish

@madjar
Copy link
Member

@madjar madjar commented Feb 13, 2020

The build seems to be failing of osx (https://logs.nix.ci/?key=nixos/nixpkgs.79941&attempt_id=07826efa-a7b1-4528-99ed-5345aba1fb30) with

In file included from /tmp/nix-build-fish-3.1.0.drv-0/fish-3.1.0/src/expand.cpp:20:
/nix/store/0xqs5h1n9avficlxbhh299bphpmnpbhk-Libsystem-osx-10.12.6/include/sys/proc.h:119:19: error: field has incomplete type 'struct itimerval'
        struct  itimerval p_realtimer;  /* Alarm timer. */
                          ^
/nix/store/0xqs5h1n9avficlxbhh299bphpmnpbhk-Libsystem-osx-10.12.6/include/sys/proc.h:119:9: note: forward declaration of 'itimerval'
        struct  itimerval p_realtimer;  /* Alarm timer. */
                ^

I found nothing relevant in the fish issues, nor in patch changing the version in homebrew. I'm not sure how to proceed.

@cole-h
Copy link
Member Author

@cole-h cole-h commented Feb 13, 2020

Unfortunately, I don't have access to any Apple devices. If someone who uses darwin could play with this to try and find out what's happening, that would be fantastic. Anybody interested in bisecting it: I can post my default.nix I used to bisect another issue previously, but I won't have access to it until this evening.

EDIT: Here is a minimal default.nix I used to nix-build when I was bisecting. Just git bisect good 3.0.2, git bisect bad 3.1.0 and go at it.

with import <nixpkgs> { };

stdenv.mkDerivation {
  name = "fish-shell";
  version = ":(";

  buildInputs = [ cmake ncurses pcre2 ];

  src = ./.;
}
@lilyball
Copy link
Member

@lilyball lilyball commented Feb 14, 2020

Interestingly, sys/proc.h on my macOS 10.15 system has #include <sys/time.h> (struct itimerval is defined in that header) but /nix/store/0xqs5h1n9avficlxbhh299bphpmnpbhk-Libsystem-osx-10.12.6/include/sys/proc.h is missing that include. That missing include is the proximate cause for the error, so perhaps we can work around it by inserting an #include <sys/time.h> at the right spot, but I'll continue investigating.

@lilyball
Copy link
Member

@lilyball lilyball commented Feb 14, 2020

The failure was introduced by fish-shell/fish-shell@fc0c39b, which removed an import of <sys/sysctl.h>, which was indirectly importing <sys/time.h>.

@lilyball
Copy link
Member

@lilyball lilyball commented Feb 14, 2020

I've filled a PR with Fish as fish-shell/fish-shell#6602. We should be able to just fetch and apply https://github.com/fish-shell/fish-shell/pull/6602.patch as a patch.

@lilyball
Copy link
Member

@lilyball lilyball commented Feb 14, 2020

Well, I suppose fetching the PR as a patch opens us up to hash changes. I'm not sure what the best solution is prior to the PR being merged; we could fetch the commit, but then if the PR's history is edited the commit isn't guaranteed to stay.

Or we could just copy the patch locally.

@cole-h
Copy link
Member Author

@cole-h cole-h commented Feb 14, 2020

I'll copy it locally, just to be safe. Thanks for tracking it down!

(forgot to add a comment above the patch in the list to explain why it's necessary, so I force-pushed again)

@cole-h cole-h force-pushed the cole-h:fish branch 2 times, most recently from 3537bd3 to b2ef01a Feb 14, 2020
@ofborg ofborg bot requested a review from ocharles Feb 14, 2020
@madjar
Copy link
Member

@madjar madjar commented Feb 14, 2020

I can confirm that this fixes the build on OSX.

@mmahut
Copy link
Member

@mmahut mmahut commented Feb 14, 2020

@GrahamcOfBorg build fish

pkgs/shells/fish/default.nix Outdated Show resolved Hide resolved
@cole-h cole-h force-pushed the cole-h:fish branch from b2ef01a to 8461450 Feb 14, 2020
@cole-h
Copy link
Member Author

@cole-h cole-h commented Feb 14, 2020

Addressed reviews, and added a few spaces to align stuff (I couldn't help myself).

pkgs/shells/fish/default.nix Outdated Show resolved Hide resolved
fish-shell 3.1.0 was just released today with many niceties (including
the ability to have bash-like temporary env vars e.g. `VAR="var1"
command` instead of needing to use `env VAR="var1" command`). To see the
full list of changes, please visit
https://github.com/fish-shell/fish-shell/releases/tag/3.1.0.
@cole-h cole-h force-pushed the cole-h:fish branch from 8461450 to be2ceb2 Feb 14, 2020
Copy link
Member

@lilyball lilyball left a comment

LGTM. This passes nix-review on darwin

https://github.com/NixOS/nixpkgs/pull/79941
1 package built:
fish
@anrddh
Copy link

@anrddh anrddh commented Feb 17, 2020

What additional work needs to get done before this can be merged?

@cole-h
Copy link
Member Author

@cole-h cole-h commented Feb 17, 2020

I believe we're just waiting on @ocharles to re-review, since he's the maintainer.

If you can't wait for that, an overlay works just fine (and is what I'm using):

# ~/.config/nixpkgs/overlays/fish.nix
self: super:

{
  fish = super.fish.overrideAttrs (old: rec {
    version = "3.1.0";

    patches = [ ];

    src = super.fetchurl {
      url =
        "https://github.com/fish-shell/fish-shell/releases/download/${version}/${old.pname}-${version}.tar.gz";
      sha256 = "0s2356mlx7fp9kgqgw91lm5ds2i9iq9hq071fbqmcp3875l1xnz5";
    };

    postInstall = "";
  });
}
Copy link
Member

@ocharles ocharles left a comment

This all LGTM, thanks for your hard work!

I should probably step down from maintainership, as you can see I'm not really doing much... maintaining. Anyone interested in stepping up?

@madjar
Copy link
Member

@madjar madjar commented Feb 18, 2020

I can't really maintain this since I'm not using it yet in my weird hybrid osx setup, but I can definitely merge it!

@madjar madjar merged commit af8d700 into NixOS:master Feb 18, 2020
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
fish on aarch64-linux Success
Details
fish on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@terlar
Copy link
Contributor

@terlar terlar commented Feb 18, 2020

Seems this broke the nixos module programs.fish (which applies a patch to the fish src):
#80423

@cole-h cole-h deleted the cole-h:fish branch Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.