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

wait-for-it: init at unstable-2020-02-05 #89493

Closed
wants to merge 2 commits into from

Conversation

silviogutierrez
Copy link

Motivation for this change

Add wait-for-it package, a zero dependency battle-tested wait to wait for a service to be available.

Please note, the repo, while very popular, does not seem to use the concept of tags, versions, or releases. So I just pinned it to the latest build from master.

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.

@silviogutierrez
Copy link
Author

Not sure what commit message to use given there is no version. init at master? use the hash?

@doronbehar
Copy link
Contributor

Hi and welcome to Nixpkgs 🎉 :).

Thank you for your contribution. Since the package you'd like to add to the collection is a pure Bash script, is there a particular reason you can't just clone the project's directory and link it to somewhere in your $PATH? I'm a bit opposed to adding such simple Bash scripts to our collection, and the community seems to support this opinion: https://discourse.nixos.org/t/we-need-more-defined-guidelines-for-package-inclusion/3592

@silviogutierrez
Copy link
Author

Hi there,

That's a great question. I think my reasoning here is that it's a very popular bash script for a very popular need: waiting for arbitrary services to start. This is especially useful when running Nix inside docker: it unlocks the ability to run more than one process in a single image.

It's such a popular task that it's even in the docker documentation, with a fragile, non-standard hack to do so: https://docs.docker.com/config/containers/multi-service_container/

My use case is the above, and just as you suggest, I'm basically using a checked-in derivation locally and doing (import ./wait-for-it) in my inputs. The file looks like this:

with import <nixpkgs> { };

stdenv.mkDerivation rec {
  name = "wait-for-it-${version}";
  version = "1.0";

  src = fetchFromGitHub {
    owner = "vishnubob";
    repo = "wait-for-it";
    rev = "c096cface5fbd9f2d6b037391dfecae6fde1362e";
    sha256 = "1k1il4bk8l2jmfrrcklznc8nbm69qrjgxm20l02k01vhv2m2jc85";
  };
    installPhase =
        ''
            mkdir -p $out/bin
            cp ./wait-for-it.sh $out/bin
        '';

  buildInputs = [];
}

So to your point: if you think of this as a "binary" that waits for services, I argue it's very useful. The language it's written in being orthogonal. In a way, no different than python packages that we have in nix that require no compilation. Why don't we use pip or clone those as well?

On the other hand, if you still oppose - which I completely understand - I'd love some guidance and ways to contribute towards making it even more lightweight to "source" shell scripts from GitHub repos without needing files like the one I pasted above. I love the nix infrastructure and plumbing, so a single one liner like importScript or something to add to repos would go a long way.

Thanks for all the work on nix. No joke, it's changed my life.

@doronbehar
Copy link
Contributor

it's a very popular bash script

Not that popular according to: https://repology.org/projects/?search=wait-for-it&maintainer=&category=&inrepo=&notinrepo=&repos=&families=&repos_newest=&families_newest=

Also, what bothers me is that it seems unmaintained according to the number of open issues and PRs.

This is especially useful when running Nix inside docker

Doesn't sound more useful for Nix vs other distros.

The language it's written in being orthogonal. In a way, no different than python packages that we have in nix that require no compilation. Why don't we use pip or clone those as well?

Because it's a pain to handle Python deps without Nix. Consider how many things buildPythonApplication does for you vs what you need from mkDerivation - You don't need to handle:

  • Dependencies.
  • Wrapping executables with the proper Python environment necessary to execute Python applications.
  • Using the right Python interpreter with the Right dependencies.

Dealing with Bash scripts is super easy compared to doing that for Python scripts without Nixpkgs' Python ecosystem. Note that mere Python scripts with no non-standard dependencies are sometimes rejected as well.

On the other hand, if you still oppose - which I completely understand - I'd love some guidance and ways to contribute towards making it even more lightweight to "source" shell scripts from GitHub repos without needing files like the one I pasted above.

Definitely we are lacking such guidelines and that was the purpose of that discourse thread. I think most people consider it easy enough to source shell scripts for these little quirks of services startups. Personal advice: If you use this script in your services deployments, why not add it as a Git submodule to your project and track it there? If you use more of Nix vs Docker, maybe use an overlay?

@silviogutierrez
Copy link
Author

So, for your popularity metric, I believe it measures projects that refer to it or depend on it somehow. It's low precisely because people are downloading and checking this into their repo. Bash does not have a standout package manager[1]. So I guess since I think nix is so useful, it should embrace the ability to package bash. The fact that the coreutils etc are already standardized inside nix is a very nice benefit.

Regarding maintainability: I guess that's subjective. It's got recent commits and a few issues triaged. But yea, it's not super active. I suppose where you see "unmaintained" I see "finished".

Regarding nix/docker: in other distros and docker image, the root images are highly specialized. Yes you can install and run node inside the python3.8 image, or vice versa. The NixOS image is amazing for having the same environment in production that you have in development. This is something fewer people realize and I plan to write about. Also totally a tangent, but I'm on a nix bender right now.[2]. So it's very easy to have your Node and Python servers inside the same docker image (negating the need for full-on orchestration).

Regarding using git submodules: I think that's a personal thing. I'm not a huge fan. And I'm on the quixotic quest to achieve single shell.nix nirvana.

So to summarize, all kind of related:

  • Long term: I think there's a great opportunity for nix to embrace bash scripts or "binaries". It can replace the million ad-hoc ways people depend on bash utilities and scripts. As we both agree, perhaps not necessarily in nixpkgs but through a simple sourceShellFromGit DSL or the like.
  • I don't see this as unmaintained, but I'm not sure if there's an objective criteria (time to response for security bugs perhaps?)
  • I didn't realize this actually required justification (which is fine, of course). I thought of it as a no-brainer. Evidently it's not, so by all means, I don't want to take up too much of your time if this is less of an addition to nix than a concession by nix.

[1] I don't really see https://github.com/bpkg/bpkg as having taken off.

[2] Images do tend to be much larger though.

{ stdenv, lib, fetchFromGitHub }:

stdenv.mkDerivation rec {
name = "wait-for-it";
Copy link
Contributor

@jtojnar jtojnar Jun 4, 2020

Choose a reason for hiding this comment

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

Suggested change
name = "wait-for-it";
pname = "wait-for-it";
version = "unstable-2020-02-05";

see https://nixos.org/nixpkgs/manual/#sec-package-naming

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but changed to

  pname = "wait-for-it-unstable";
  version = "2020-02-05";

so that it matches the Nix notion of derivation name/version:

nix-repl> builtins.parseDrvName "wait-for-it-unstable-2020-02-05"
{ name = "wait-for-it-unstable"; version = "2020-02-05"; }

Copy link
Author

Choose a reason for hiding this comment

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

Would this affect say, nix-shell -p? I'd assume users what the least surprise here, and typing nix-shell -p wait-for-it-unstable is certainly more obscure than nix-shell -p wait-for-it.

Please let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

-p uses attribute names so it behaves correctly. Only nix-env without -A will be problematic. I suggested using yyyy-mm-dd-unstable version format to sidestep this elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also #88023 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@jtojnar I think the solution depends on the situation. Quite a few packages end up in Nixpkgs before upstream have ever made a versioned release. If the upstream then makes a release 1.0 we'd typically want to track that but then it is unfortunate to have, for example, foo-2020-06-10-unstable since 2020-06-10-unstable > 1.0 using version comparison. Then nix-env --upgrade would silently leave the old "unstable" version as-is. If instead the package name changes from foo-unstable to foo perhaps you'd at least get an error?

If the intent is to swap the package over to a stable version if one ever appears then perhaps some thing like foo-0.0.0-nix20200610 would be more suitable? And foo-unstable-2020-06-10 could then be reserved for packages really intended to forever track the unstable branch of foo.

@jtojnar
Copy link
Contributor

jtojnar commented Jun 4, 2020

I think that until we have inclusion policy this should qualify.

  • not abandoned
  • low importance but we can still give it a chance
  • low complexity so there is lower chance of breaking
  • someone wants it enough to package it

We can always just remove it when it breaks and nobody steps in to fix it.

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Commit should be named following our guidelines.

Note a Debian package exists for it.

pkgs/tools/networking/wait-for-it/default.nix Outdated Show resolved Hide resolved
@silviogutierrez
Copy link
Author

Commit should be named following our guidelines.

Note a Debian package exists for it.

  1. Should I squash all commits (since I have to revise this PR) into a new one with the right formatting? Or should I just rename the PR title?
  2. Since this package doesn't have versioning, is using @jtojnar 's unstable-2020-02-05 the correct version?

Thanks for your help! Still learning.

@doronbehar
Copy link
Contributor

Your PR should have eventually two commits:

maintainers: add <me>
<package-name>: init at unstable-2020-02-05

Updating the PR title is nice but it's not a must.

@silviogutierrez
Copy link
Author

Hi all, I think this is ready to go. Please let me know and many thanks for all your help.

@balsoft
Copy link
Member

balsoft commented Jun 18, 2020

Hi!

First of all, thank you for the contribution!

Sorry for the nitpick, but there should be 2 commits: one adding you as a maintainer and another adding the package. You can just run this:

git reset
git add maintainers
git commit -m "maintainers: add silviogutierrez"
git add pkgs
git commit -m "wait-for-it: init at unstable-2020-02-05"

in a clean checkout.

@silviogutierrez
Copy link
Author

@balsoft

Hi there,

Not nitpicking at all! I went off of this random PR and they only had 1 commit, so I figured maybe that was ok: #57893

I restructured it as two commits. For posterity, these were the steps to do it cleanly while staying on the branch:

git reset --hard origin/master
git checkout origin/wait-for-it -- .
git reset
git add maintainers
git commit -m "maintainers: add silviogutierrez"
git add pkgs
git commit -m "wait-for-it: init at unstable-2020-02-05"
git push -f

Addressing your other comments inline. Thanks for the feedback.

@silviogutierrez
Copy link
Author

@balsoft : all done. Thanks again.

@FRidh
Copy link
Member

FRidh commented Jun 19, 2020

Setting phases should be avoided and is really only for generic builders. Much better to pass things like dontBuild = true;.

@silviogutierrez
Copy link
Author

hi @balsoft , is there anything else needed for this to go in? Thanks again for your help.

@dasJ
Copy link
Member

dasJ commented Jun 28, 2020

Well it does have dependencies: netcat. It should probably be wrapped to include nc in its $PATH.

EDIT: Sorry, this is only required when running from busybox.

{ stdenvNoCC, lib, fetchFromGitHub }:

stdenvNoCC.mkDerivation rec {
pname = "wait-for-it-unstable";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pname = "wait-for-it-unstable";
pname = "wait-for-it";

rev = "c096cface5fbd9f2d6b037391dfecae6fde1362e";
sha256 = "1k1il4bk8l2jmfrrcklznc8nbm69qrjgxm20l02k01vhv2m2jc85";
};
phases = [ "unpackPhase" "installPhase" "fixupPhase" ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
phases = [ "unpackPhase" "installPhase" "fixupPhase" ];

Comment on lines +21 to +27
meta = {
homepage = "https://github.com/vishnubob/wait-for-it";
description =
"Pure bash script to test and wait on the availability of a TCP host and port";
license = lib.licenses.mit;
maintainers = [ lib.maintainers.silviogutierrez ];
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
meta = {
homepage = "https://github.com/vishnubob/wait-for-it";
description =
"Pure bash script to test and wait on the availability of a TCP host and port";
license = lib.licenses.mit;
maintainers = [ lib.maintainers.silviogutierrez ];
};
meta = with lib; {
homepage = "https://github.com/vishnubob/wait-for-it";
description = "Pure bash script to test and wait on the availability of a TCP host and port";
license = licenses.mit;
maintainers = with maintainers; [ silviogutierrez ];
};

@SuperSandro2000 SuperSandro2000 changed the title Add wait-for-it package wait-for-it: init at unstable-2020-02-05 Nov 29, 2020
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 89493 run on x86_64-linux 1

1 package built:
  • wait-for-it

@stale
Copy link

stale bot commented Jul 2, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 2, 2021
@SuperSandro2000
Copy link
Member

Closing due to inactivity from author.

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

9 participants