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

Jump to bottom

hover: init at 0.43.0 #80075

Merged
merged 1 commit into from Dec 12, 2020
Merged

hover: init at 0.43.0 #80075

merged 1 commit into from Dec 12, 2020

Conversation

ericdallo
Copy link
Member

Motivation for this change

Add hover command to be able to run flutter mobile apps on desktop.

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.

This was referenced Feb 14, 2020
Merged
Closed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/116

@ofborg ofborg bot requested a review from ehmry February 14, 2020 00:45
@dtzWill
Copy link
Member

dtzWill commented Feb 14, 2020

I don't know much about flutter or hover, but gave it a build and noticed that ./result-bin/bin/app wants libflutter_engine.so but can't find it -- does a path needed to be added to its RPATH or something?

@ericdallo
Copy link
Member Author

Hi @dtzWill, thanks for testing it :)
It looks like this fixed this app binary, the correct generated binary is only hover

@ofborg ofborg bot requested a review from kalbasit February 14, 2020 12:59
jonringer
jonringer reviewed Feb 14, 2020
View reviewed changes
pkgs/development/tools/hover/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/hover/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/hover/default.nix Outdated Show resolved Hide resolved
@jonringer
Copy link
Contributor

the deps.nix is no longer necessary.

and please squash commits

@ericdallo
Copy link
Member Author

Thanks @jonringer!
Done

@kalbasit
Copy link
Member

@GrahamcOfBorg build hover

jonringer
jonringer reviewed Feb 14, 2020
View reviewed changes
pkgs/development/tools/hover/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/hover/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/hover/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from kalbasit February 14, 2020 22:34
@ericdallo
Copy link
Member Author

@GrahamcOfBorg build hover

@kalbasit
Copy link
Member

@GrahamcOfBorg build hover

@jonringer
Copy link
Contributor

is this ready to be reviewed? looks close

@ericdallo
Copy link
Member Author

@jonringer Unfortunately I was not able to get it working without using the hover docker param, this is where I stuck. But I'm using the docker version (this PR changed files) on my work for months and it's working ok.

@jonringer
Copy link
Contributor

If it works well, then I think it's able to be merged.

Smaller closures are a good goal, but it seems like you did a solid attempt to try and reduce it.

Going to let some time for people to comment if they have any strong objections, then I'll come back around and do a review + merge.

@ericdallo
Copy link
Member Author

Thank you @jonringer!

thiagokokada pushed a commit to nubank/nixpkgs that referenced this pull request Sep 2, 2020
Put hover inside a buildFHSUserEnv
5e54789 
Based on this PR: NixOS/nixpkgs#80075

I exported the `go` and `gcc` packages so it was available inside the
FHS env and also fixed the X11 libraries, and this seems to do the
trick.

There is no more reason to wrapper with CPATH, and the only reason we
keep LD_LIBRARY_PATH is because hover is built outside the
buildFHSUserEnv so it wouldn't found the libraries otherwise.
@thiagokokada thiagokokada mentioned this pull request Sep 2, 2020
Merged
@thiagokokada
Copy link
Contributor

Just to help having this merge, we finally got a way to make hover (without docker) running reliable in NixOS:

{ lib
, buildGoModule
, buildFHSUserEnv
, dejavu_fonts
, pkgconfig
, fetchFromGitHub
, stdenv
, roboto
, writeScript
, xorg
, libglvnd
, addOpenGLRunpath
, makeWrapper
, gcc
, go
, flutter
}:

let
  pname = "hover";
  version = "0.43.0";

  libs = with xorg; [
    libX11.dev
    libXcursor.dev
    libXext.dev
    libXi.dev
    libXinerama.dev
    libXrandr.dev
    libXrender.dev
    libXfixes.dev
    libXxf86vm
    libglvnd.dev
    xorgproto
  ];
  hover = buildGoModule rec {
    inherit pname version;

    meta = with stdenv.lib; {
      description = "A build tool to run Flutter applications on desktop";
      homepage = "https://github.com/go-flutter-desktop/hover";
      license = licenses.bsd3;
      platforms = platforms.linux ++ platforms.darwin;
    };

    subPackages = [ "." ];

    vendorSha256 = "1wr08phjm87dxim47i8449rmq5wfscvjyz65g3lxmv468x209pam";

    src = fetchFromGitHub {
      rev = "v${version}";
      owner = "go-flutter-desktop";
      repo = pname;
      sha256 = "0iw6sxg86wfdbihl2hxzn43ppdzl1p7g5b9wl8ac3xa9ix8759ax";
    };

    nativeBuildInputs = [ addOpenGLRunpath makeWrapper ];

    buildInputs = libs;

    checkRun = false;

    patches = [
      ./fix-assets-path.patch
    ];

    postPatch = ''
      sed -i 's|@assetsFolder@|'"''${out}/share/assets"'|g' internal/fileutils/assets.go
    '';

    postInstall = ''
      mkdir -p $out/share
      cp -r assets $out/share/assets
      chmod -R a+rx $out/share/assets

      wrapProgram "$out/bin/hover" \
      --prefix LD_LIBRARY_PATH : ${stdenv.lib.makeLibraryPath libs}
    '';

    postFixup = ''
      addOpenGLRunpath $out/bin/hover
    '';
  };

in
buildFHSUserEnv rec {
  name = pname;
  targetPkgs = pkgs: [
    dejavu_fonts
    flutter
    gcc
    go
    hover
    pkgconfig
    roboto
  ] ++ libs;

  runScript = "hover";
}

If @ericdallo can update this PR we probably can get his merged.

@ericdallo
Copy link
Member Author

Yes, thanks for all the help @thiagokokada, I'll update it

@ericdallo ericdallo force-pushed the hover-flutter branch 2 times, most recently from c47056a to 91cea53 Compare September 18, 2020 02:28
@thiagokokada
Copy link
Contributor

@ericdallo Can you update the commit message? You are using version 0.4.3, not 2020-01-03.

@ericdallo
Copy link
Member Author

@GrahamcOfBorg build hover

@ericdallo
Copy link
Member Author

After 7 months and the help of @thiagokokada I think we are good to go here @jonringer 🎉

thiagokokada
thiagokokada approved these changes Sep 18, 2020
View reviewed changes
pkgs/development/tools/hover/default.nix
homepage = "https://github.com/go-flutter-desktop/hover";
license = licenses.bsd3;
platforms = platforms.linux ++ platforms.darwin;
maintainers = [ maintainers.ericdallo maintainers.thiagokokada];
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 .

@ericdallo ericdallo mentioned this pull request Sep 21, 2020
Closed
@thiagokokada
Copy link
Contributor

@jonringer Can we have this PR merged?

@zimbatm zimbatm changed the title hover: init 2020-01-03 hover: init 0.43.0 Dec 12, 2020
@zimbatm zimbatm changed the title hover: init 0.43.0 hover: init at 0.43.0 Dec 12, 2020
@zimbatm zimbatm merged commit 7bb9e50 into NixOS:master Dec 12, 2020
@ericdallo ericdallo deleted the hover-flutter branch December 12, 2020 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer approved these changes

@thiagokokada thiagokokada thiagokokada approved these changes

@ehmry ehmry Awaiting requested review from ehmry

@kalbasit kalbasit Awaiting requested review from kalbasit

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ericdallo @nixos-discourse @dtzWill @jonringer @kalbasit @zimbatm @thiagokokada