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.

Sign up for GitHub

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

Adding the sed replacement sd, written in rust #54761

Closed
wants to merge 1 commit into from
Closed

Adding the sed replacement sd, written in rust #54761

wants to merge 1 commit into from

Conversation

abhi18av
Copy link
Member

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • [X ] 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 nox --run "nox-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.

@abhi18av abhi18av changed the title updated the text message and configs Adding the sed replacement sd, written in rust Jan 28, 2019
@abhi18av abhi18av mentioned this pull request Jan 28, 2019
Closed
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Jan 28, 2019
@etu
Copy link
Contributor

etu commented Jan 28, 2019

Without actually looking at the code, you would need to put the file in it's own directory and not just as default.nix in the misc directory. I haven't made up my mind if misc is the right place, but it probably is. But at least it needs to be in a directory named sd in there.

Secondly you need to add a line to the all-packages.nix to reference the derivation.

And thirdly you should amend your commit and push it with a message like: sd: init at 0.4.3 to follow the contributing guidelines for commit messages.

@etu
Copy link
Contributor

etu commented Jan 28, 2019

And another thing, you have to add yourself to the file maintainers/maintainer-list.nix as well

@abhi18av abhi18av mentioned this pull request Feb 1, 2019
Merged
@abhi18av
Copy link
Member Author

abhi18av commented Feb 1, 2019

I see, thanks @etu

I've added myself as a maintainer and I'll be re-working this PR soon.

@fpletz fpletz added the 2.status: work-in-progress This PR isn't done label Feb 3, 2019
@infinisil infinisil added the 8.has: package (new) This PR adds a new package label Mar 30, 2019
@Shou
Copy link
Contributor

Shou commented Apr 2, 2019

I wonder if pkgs/tools/text would be a better place? That's where ripgrep, gnused, the_silver_searcher is.

liff
liff reviewed Apr 16, 2019
View reviewed changes
pkgs/tools/misc/default.nix
name = "sd-${version}";
version = "0.4.3";

cargoSha256 = "08zzn3a32xfjkmpawcjppn1mr26ws3iv40cckiz8ldz4qc8y9gdh";
Copy link
Contributor

Choose a reason for hiding this comment

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

This hash is from the exa package. Should be changed to the correct value that you can get from a failed build, like described in the manual.

pkgs/tools/misc/default.nix
owner = "chmln";
repo = "sd";
rev = "v${version}";
sha256 = "0jy11a3xfnfnmyw1kjmv4ffavhijs8c940kw24vafklnacx5n88m";
Copy link
Contributor

Choose a reason for hiding this comment

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

This hash is also from the exa package. You should be able to get the correct hash with this command:

nix run nixpkgs.nix-prefetch-git -c nix-prefetch-git https://github.com/chmln/sd 0.4.3

pkgs/tools/misc/default.nix
};

nativeBuildInputs = [ cmake pkgconfig perl ];
buildInputs = [ zlib ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither of nativeBuildInputs or buildInputs are needed for sd.

pkgs/tools/misc/default.nix
libiconv darwin.apple_sdk.frameworks.Security ]
;

postInstall = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also from exa and should be removed.

pkgs/tools/misc/default.nix
'';

# Some tests fail, but Travis ensures a proper build
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed for sd.

pkgs/tools/misc/default.nix
with rustPlatform;

buildRustPackage rec {
name = "sd-${version}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use pname = "sd" instead.

@dywedir
Copy link
Member

dywedir commented Apr 27, 2019 •
edited
Loading

Thanks for the PR!
Superseded by #60241

@dywedir dywedir closed this Apr 27, 2019
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liff liff liff left review comments

Assignees
No one assigned
Labels
2.status: work-in-progress This PR isn't done 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@abhi18av @etu @Shou @dywedir @liff @fpletz @infinisil @GrahamcOfBorg @Janik-Haag