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
[WIP] Add (optional) license white/blacklisting #5892
Conversation
assert (unfreeOrBroken == "Unfree" | ||
|| unfreeOrBroken == "Broken" | ||
|| unfreeOrBroken == "AllowedLicense" | ||
|| unfreeOrBroken == "BlacklistedLicense"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use
builtins.elem x xs
Return true if a value equal to x occurs in the list xs, and false otherwise.
to make this shorter. Not sure if it is better from a memory or speed standpoint though.
So right now if you want to allow a specific unfree license, you need to set allowUnfree and blacklist all the ones you don't like. Wouldn't it be better to let allowedLicenses be empty by default and work as a whitelist? |
So, I'm done for today. I would like to hear some more feedback on this and whether this is a good idea or not, before putting effort in history-cleanup. But now I need some sleep. My idea was not to change the type of the |
Behavior I'd expect:
|
@wmertens Of course, that's what I'd like too, but how should they play together? If I do not allow unfree licenses but whitelist the Oracle one and then blacklist it again... is it allowed or not? If I both black- and whitelist GPLv3, is it allowed or not? |
I'm just jumping in here. "If I both black- and whitelist GPLv3, is it allowed or not?" -> That's a configuration error that can be caught by an assertion (i.e. it's not allowed). |
Would be a good idea to do it this way, yes. |
Indeed, assert that blacklist and whitelist are mutually exclusive. On Thu Jan 22 2015 at 9:04:26 AM Matthias Beyer notifications@github.com
|
I would do it this way: (Pseudocode ahead!)
|
Not quite - some companies do not want to use e.g. free GPLv3 software so On Thu Jan 22 2015 at 12:27:26 PM Matthias Beyer notifications@github.com
|
@wmertens look at my latest patch. It does it this way:
I will patch |
@@ -16,6 +16,12 @@ let | |||
|
|||
allowUnfree = config.allowUnfree or false || builtins.getEnv "NIXPKGS_ALLOW_UNFREE" == "1"; | |||
|
|||
# Allowed licenses, defaults to no licenses | |||
whitelistedLicenses = config.allowedLicenses or []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to see config.allowedLicenses
renamed...
I'd expect if hasBlacklistedLicense attrs then
throw...
else if (! hasWhitelistedLicense attrs) and (hasDeniedUnfreeLicense attrs) then
throw...
else |
That's exactly what's implemented right now, isn't it? The mutual-exclusive check is added as well, of course. |
if mutualExclusive whitelistedLicenses blacklistedLicenses then | ||
throw '' | ||
Package blacklist and whitelist are not mutual exclusive. | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That semicolon is unneeded (and I'm surprised it even evaluates)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skip the CI.
You don't test locally? Because I think you didn't define |
I don't test locally because I do not want to test after every change. I want to test once, but do it right, after we agreed on the actual default behaviour. |
well the patch looks good to me so start testing 😉 |
@wmertens Okay, can you tell me a way how you'd like this to be tested? |
@matthiasbeyer we don't have unit tests for configuration afaik so simply clone your tree locally and try to evaluate few configurations with packages that have proper and denied licenses... To evaluate, export |
I was able to run Edit: I just blacklisted all GPLv2.* licenses, and it does not warn or fail that the linux kernel is installed... so this works only when installing new packages, I guess. Same applies to the normal |
With GPLv2 blacklisted and a new |
Indeed. The -v shows that it is using your copy of the tree? Start On Fri Jan 23 2015 at 3:38:13 PM Matthias Beyer notifications@github.com
|
Somehow, setting the environment variable did not work. Doing nixos-rebuild dry-run -v -I nixpkgs=~/nixpkgs Fails because the linux kernel has a blacklisted license... yay! I will do more debugging now... |
I see a spurious ';'. Also, can you squash everything into one commit? |
a4e955f
to
9a45746
Compare
I guess the |
Can you remove the trace calls, that will also fix the travis build failure. |
@wmertens I'd really love to have these kind of notification, as the users can actually see that there are things which can not be filtered. Why are there packages without a name? |
Well, trace is for debugging, it's not generally used in nixpkgs at this point in time. If you want to let people know which ones are allowed through, implement a |
assert (builtins.elem unfreeOrBroken [ "Unfree" | ||
"Broken" | ||
"AllowedLicense" | ||
"BlacklistedLicense" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not using BlacklistedLicense? Shouldn't AllowedLicense be taken out and instead use BlacklistedLicense?
92cacac
to
7eca1f3
Compare
Rebased onto latest master as I guess the latest breakage was not caused by my PR... |
throwEvalHelp "Unfree" "has an unfree license" | ||
if !(mutualExclusive whitelistedLicenses blacklistedLicenses) then | ||
throw '' | ||
Package blacklist and whitelist are not mutual exclusive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more things : could you name the attributes in this error message and can you squash again?
@wmertens Can you tell me what causes the travis build failure? Is it caused by one of my patches, actually? |
7eca1f3
to
5245370
Compare
No, master is broken because of the ruby or python work. However the On Sat, Jan 24, 2015, 3:27 PM Matthias Beyer notifications@github.com
|
Alright it looks good to merge now but can you squash it into a single commit? |
I would actually like to keep the "Remove trace calls by outcommenting" commit as seperate commit. If further work needs to be done in this area, one can simply revert this bit and get this going. If you really want me to squash it I'll do this, though. |
Yes squash please, uncommenting the calls is easy enough. |
5245370
to
aca361f
Compare
Awesome, thanks! You might want to tell the mailing list about it :-) |
Add (optional) license white/blacklisting
in | ||
if !allowUnfree && isUnfree (lib.lists.toList attrs.meta.license or []) && !allowUnfreePredicate attrs then | ||
throwEvalHelp "Unfree" "has an unfree license" | ||
if !(mutualExclusive whitelistedLicenses blacklistedLicenses) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be lifted out to prevent it from being evaluated on every call to mkDerivation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm should have caught that sorry. @matthiasbeyer feel like tackling this or shall I do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wmertens Would be nice if you'd do it, I'm in the exams phase now and don't know how long it would take if I'd do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0feb19b
[WIP]
This adds optional white- or blacklisting for licenses from the users configuration as I proposed on the ML.
Please tell me what you guys think about this.
(I hope the
[ci skip]
thing worked here... as this is wip)