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

grip-0.8: indexed grip #60182

Merged
merged 1 commit into from Nov 14, 2019
Merged

grip-0.8: indexed grip #60182

merged 1 commit into from Nov 14, 2019

Conversation

tex
Copy link
Contributor

@tex tex commented Apr 24, 2019

Motivation for this change

New CLI application for indexed grepping. Fast. Useful. Can be cross-compiled for Windows.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
@tex tex force-pushed the grip branch 3 times, most recently from 3fab489 to 1b91817 Compare April 25, 2019 20:57
@tex
Copy link
Contributor Author

tex commented Apr 25, 2019

Is there a clean way to solve project names clash?

@aanderse
Copy link
Member

aanderse commented Apr 25, 2019

Oh... I see what is going on... never realized there was a grip already. I feel like grip the gnome application has likely existed much longer than grip the search+indexing tool. I guess you should rename your app? 🤷‍♂️

ping @jtojnar as a gnome expert and all around great person 😄

@jtojnar
Copy link
Contributor

jtojnar commented Apr 25, 2019

I would suggest choosing grip-search or gripgen attribute for this package. Then remove the old abandoned package. And only rename this package after some cycles.

@tex
Copy link
Contributor Author

tex commented Apr 28, 2019

It is not my application :) I just found it useful in my job. Named attribute to grip-search. Probably no need to remove existing grip - at most if anyone use both there will be clash in names (grip), but I do not think too many people will want to use grip-search and grip together.

pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

I'm sorry this I haven't got back to this until now. This dropped off my radar.

On top of the suggestions I have made you should probably rename the file from grip.nix to grip-search.nix so it matches the attribute name.

After you're done with the changes please squash all commits into a single commit with message grip-search: init at 0.8.

pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip/default.nix Outdated Show resolved Hide resolved
@tex
Copy link
Contributor Author

tex commented Nov 7, 2019

@aanderse Hi, please see changes.

@tex tex changed the title grip-v0.7: indexed grip grip-0.8: indexed grip Nov 8, 2019
@tex
Copy link
Contributor Author

tex commented Nov 11, 2019

Hi, is there anything else needed to to to get this merged?

@tex tex requested a review from aanderse November 11, 2019 07:45
pkgs/tools/text/grip-search/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip-search/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip-search/default.nix Outdated Show resolved Hide resolved
pkgs/tools/text/grip-search/default.nix Show resolved Hide resolved
@Azulinho
Copy link
Contributor

@JohnAZoidberg happy to be left out,
My Cmake skills are not that great,I'd rather focus on stuff I know well.

@tex
Copy link
Contributor Author

tex commented Nov 12, 2019 via email

@tex
Copy link
Contributor Author

tex commented Nov 14, 2019

I tested crosscompile when I first submitted this merge request, but trying it now (with updated nixpkgs) leads to:

[1] milan@nixos> nix-build '<nixpkgs>' -A pkgsCross.mingwW64.meson                                                   /etc/nixos/nixpkgs
warning: Nix search path entry '/etc/nixos/overlay' does not exist, ignoring
error: attribute 'targetPlatform' missing, at /etc/nixos/nixpkgs/pkgs/development/tools/build-managers/meson/default.nix:70:17
(use '--show-trace' to show detailed location information)

I submitted this as bug #73420

@tex
Copy link
Contributor Author

tex commented Nov 14, 2019

I tested crosscompile when I first submitted this merge request, but trying it now (with updated nixpkgs) leads to:

[1] milan@nixos> nix-build '<nixpkgs>' -A pkgsCross.mingwW64.meson                                                   /etc/nixos/nixpkgs
warning: Nix search path entry '/etc/nixos/overlay' does not exist, ignoring
error: attribute 'targetPlatform' missing, at /etc/nixos/nixpkgs/pkgs/development/tools/build-managers/meson/default.nix:70:17
(use '--show-trace' to show detailed location information)

I submitted this as bug #73420

Funny, it works, I tried to cross compile grip (the gui application) which failed. grip-search works.
See originaly it was named grip (and original grip removed), then I renamed to grip-search :D

@JohnAZoidberg
Copy link
Member

Works fine on NixOS and I'm happy with the derivation.

When I build it for Windows with: nix-build -A pkgsCross.mingwW64.grip-search, copy result/bin/* to Windows 10 and run it, this happens:

image

Did you perform other steps?

@JohnAZoidberg JohnAZoidberg merged commit c96e556 into NixOS:master Nov 14, 2019
@tex
Copy link
Contributor Author

tex commented Nov 15, 2019

Oh, I tested that crosscompilation when I first created this merge request.
This must be dependency created by #73195 . Fun thing is that I modified grip on master to remove dependency on multithreading and other C++11 things that the #73195 enabled in Nix when crosscompiling. Guess you need to install that missing library into system, it is not dll created by grip.

@tex tex deleted the grip branch November 15, 2019 20:57
@tex
Copy link
Contributor Author

tex commented Nov 15, 2019

Or is it problem of #73195 that it doesn't provide all in runtime required libraries in resulting output?

@Ericson2314
Copy link
Member

@tex We have always lacked a sufficiently good automated mechanism for copying all of the required DLLs into the bin/ directory. But if you build nix-build '<nixpkgs>' -A pkgsCross.mingwW64.threadsCross you can get the DLL and copy it yourself.

dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Nov 16, 2019
grip-0.8: indexed grip

(cherry picked from commit c96e556)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants