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

datefudge: work correctly even if GNU date is not in PATH #94045

Merged
merged 3 commits into from Nov 1, 2021

Conversation

KAction
Copy link
Contributor

@KAction KAction commented Jul 28, 2020

Examples in manual assumes advanced features from date(1) like "last
Friday", which only available in GNU coreutils version of date(1)
utility. Without this patch, most examples from datefudge(1) manual will
fail in busybox environment, which is confusing.

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.

@KAction
Copy link
Contributor Author

KAction commented Oct 7, 2020

@Ma27 @leenaars Ping? (extracted list of previous contributors from git-log)

@dasJ
Copy link
Member

dasJ commented Nov 7, 2020

Why not go for a patchless approach and use wrapProgram?

@SuperSandro2000
Copy link
Member

I agree with @dasJ, please wrap the program.

@KAction
Copy link
Contributor Author

KAction commented Dec 15, 2020

@SuperSandro2000 @dasJ Applied changes.

@@ -1,4 +1,4 @@
{ stdenv, fetchgit, fetchpatch }:
{ stdenv, lib, fetchgit, fetchpatch, makeWrapper, coreutils ? null }:
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we just always pass in coreutils if this is possible. It would make this file less complex and coreutils is likely already downloaded anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On build system -- yes, coreutils is part of stdenv. On host system -- not necessary.

@SuperSandro2000
Copy link
Member

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

1 package built:
  • datefudge

pkgs/tools/system/datefudge/default.nix Outdated Show resolved Hide resolved
pkgs/tools/system/datefudge/default.nix Outdated Show resolved Hide resolved
pkgs/tools/system/datefudge/default.nix Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Jul 20, 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 20, 2021
Examples in manual assumes advanced features from date(1) like "last
Friday", which only available in GNU coreutils version of date(1)
utility. Without this patch, most examples from datefudge(1) manual will
fail in busybox environment, which is confusing.
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 29, 2021
@KAction
Copy link
Contributor Author

KAction commented Oct 29, 2021

Any chance to merge it as I wrote it? I still maintain that avoiding unnecessary dependency worth two conditional statements. @SuperSandro2000

@SuperSandro2000 SuperSandro2000 merged commit f538c07 into NixOS:master Nov 1, 2021
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

3 participants