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

remind: Improve usage on macOS and Windows #67870

Merged
merged 1 commit into from Sep 9, 2019
Merged

remind: Improve usage on macOS and Windows #67870

merged 1 commit into from Sep 9, 2019

Conversation

sorbits
Copy link
Contributor

@sorbits sorbits commented Sep 1, 2019

Motivation for this change

The default version of remind has a 5 second delay for each invocation when running on macOS or Windows.

Things done

This patch removes the delay.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Sep 1, 2019
@ofborg ofborg bot requested review from 7c6f434c and KoviRobi September 1, 2019 07:58
@sorbits sorbits changed the title Patch remind source to improve usage on macOS and Windows remind: Improve usage on macOS and Windows Sep 1, 2019
@KoviRobi
Copy link
Contributor

KoviRobi commented Sep 2, 2019

Functionally, your proposed changes work, though not sure you need to remove the call to sleep() when you remove the call to rkrphgvba() (rot13 for execution), whose purpose is to display a banner saying the author would rather you didn't use the software on Windows/Apple and wait 5 seconds.

While this change goes against the author's intentions, I guess we are allowed to make these changes (and it is something Homebrew for Mac does too)

@sorbits
Copy link
Contributor Author

sorbits commented Sep 2, 2019

[…] not sure you need to remove the call to sleep() when you remove the call to rkrphgvba() […]

You are correct, I have updated the patch.

While this change goes against the author's intentions, I guess we are allowed to make these changes (and it is something Homebrew for Mac does too)

I was hesitant about this PR, but remind is not useful on macOS nor Windows without it, and the freedom the author wish to protect, is exactly the freedom that grants us the right to use their unencumbered software on a platform of our choosing.

Ideally though, the author would have made an option or environment variable to suppress the banner and delay, then there would be no need for this patch, and their message would reach a wider audience.

@matthewbauer
Copy link
Member

matthewbauer commented Sep 6, 2019 •
edited

macports has a patch for it here:

https://github.com/macports/macports-ports/blob/master/textproc/remind/files/patch-remove-apple-nag.patch

Might be better to just use that if it still applies.

Also make sure to update platforms to include macOS as well.

@sorbits
Copy link
Contributor Author

sorbits commented Sep 6, 2019 •
edited

Might be better to just use that if it still applies.

Personally I much prefer the call to substituteInPlace because it’s a minimal change yet enough to remove the banner with delay, and given the nag function’s name (rkrphgvba) it’s unlikely to match something it shouldn’t.

Also make sure to update platforms to include macOS as well.

Thanks, I hadn’t noticed this, as I had enabled unsupported platforms in my config (due to another package that didn’t support macOS).

Force-pushed a commit where platform is now set to unix.

matthewbauer
matthewbauer reviewed Sep 6, 2019
View reviewed changes
pkgs/tools/misc/remind/default.nix Outdated
@@ -21,6 +21,14 @@ in stdenv.mkDerivation {
nativeBuildInputs = optional tkremind makeWrapper;
propagatedBuildInputs = tclLibraries;

patchPhase = ''
Copy link
Member

Choose a reason for hiding this comment

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

Actually this should be put in postPatch below. Otherwise you end up overriding any patch files that were provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest push (and tested that it still works as expected).

@matthewbauer matthewbauer self-requested a review September 6, 2019 20:51
@sorbits
Patch remind source to work on macOS
c43a0e3 
The default version of the source will show a banner on macOS and Windows which tells the user that the author would rather that they didn’t use remind on Apple or Microsoft products, and then sleep for 5 seconds.

There is no way for the user to remove this obstacle other than patch the source and recompile, which is also what other package managers do.

This commit also changes supported platform from linux to unix.
@matthewbauer matthewbauer merged commit 49d7cc2 into NixOS:master Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

@KoviRobi KoviRobi Awaiting requested review from KoviRobi

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sorbits @KoviRobi @matthewbauer @Janik-Haag