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

parallel: add disableNagMessage #110633

Merged
merged 1 commit into from Oct 14, 2021
Merged

parallel: add disableNagMessage #110633

merged 1 commit into from Oct 14, 2021

Conversation

tomberek
Copy link
Contributor

@tomberek tomberek commented Jan 23, 2021

Motivation for this change

Concern about the nag screen as well as GPL issues.

See #110584
Let's break the problem down into it's components.

Nag

This is a usability concern for some people and for some automation.

GPLv3

I don't think Nixpkgs should get into the habit of "fixing" the potential license issue by patching this out completely. My personal opinion is that it is a bit non-standard, but is technically okay. Regardless, the Nix-community is not a compliance organization, nor is in a good position to enforce this. The author and the GNU organization seems to claim there is no conflict. I recommend this be brought up with them directly. Packaging systems hiding the potential issue from users by default also seems odd.

Things done

adds an option for Nix users to disable the citation request by adding a --will-cite to the wrapped parallel call.

Potential issues
  • some users may want this without the extra wrapping (this would mean a patch)
  • some may want to disable the nag, but also not use the extra perl pacakges (this would mean they can set extraPerlPackages to [])
Checklist
  • 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.

@tomberek
Copy link
Contributor Author

disableNagMessage -> willCite? Calling it a nag has a negative connotation.

@tomberek tomberek force-pushed the parallel-nag branch 2 times, most recently from 73a70d6 to c1caf3f Compare January 23, 2021 23:56
@Profpatsch
Copy link
Member

👍 this looks like a good low-overhead solution, since there is already a wrapper anyway.

@Profpatsch
Copy link
Member

only thing I would test whether parallel is okay with getting passed --will-cite twice, in order to not break existing calls that include it. We might have to deduplicate it.

@cole-h
Copy link
Member

cole-h commented Jan 24, 2021

I don't know how to use parallel, but running echo "echo asdf; echo fdsa" | parallel --will-cite --will-cite doesn't make it angry.

@tomberek tomberek marked this pull request as ready for review January 24, 2021 19:58
@ole-tange
Copy link

ole-tange commented Jan 27, 2021

It seems there is some confusion as to:

Please read: https://git.savannah.gnu.org/cgit/parallel.git/tree/doc/citation-notice-faq.txt

Especially the last section is relevant.

@tomberek
Copy link
Contributor Author

tomberek commented Jan 27, 2021

@ole-tange Is the mechanism in this PR acceptable? It is false by default and requires positive action by a user to remove the notice, similar to someone setting their own alias or running --citation themselves. (perhaps a bit more documentation needed for the option? perhaps showing the citation in a Nix trace?)

or just not apply and maintain current behavior?

@ole-tange
Copy link

@ole-tange Is the mechanism in this PR acceptable?

If the user by default is presented with the citation notice, it is fine.

The fundamental issue is to make sure the users are aware, that citations are what (indirectly) funds future development. If this can be accomplished in a more effective way, it is OK.

@Profpatsch
Copy link
Member

If the user by default is presented with the citation notice, it is fine.

So I guess this goes against disabling the nag message by default, per the author’s (@ole-tange) request.

@Profpatsch Profpatsch closed this Feb 11, 2021
@tomberek
Copy link
Contributor Author

If the user by default is presented with the citation notice, it is fine.

So I guess this goes against disabling the nag message by default, per the author’s (@ole-tange) request.

My impression is that this patch is in accordance to the author's request. By default, the package will present the citation message. Only by specifically installing parallel-full and overriding the default willCite option will the message be silenced. This is a similar level of action as a script to run touch ~/.parallel/will-cite or to alias parallel. This would only be done by someone who is already familiar with the requests and understands what that option means. It is also similar to our policy with config.allowUnfree or CUDA support in various packages.

I also want to note that I do not particularly believe this proposal is necessary [I am starting to think it would be convenient in certain automation scenarios], but it is a compromise between various positions.

@tomberek tomberek reopened this Feb 11, 2021
@stale
Copy link

stale bot commented Aug 13, 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 Aug 13, 2021
@Mic92 Mic92 merged commit b0fcb83 into NixOS:master Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants