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

python3Packages.adblock: init at 0.4.0 #108271

Merged
1 commit merged into from Jan 4, 2021
Merged

python3Packages.adblock: init at 0.4.0 #108271

1 commit merged into from Jan 4, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 2, 2021 •
edited by ghost

This packages python bindings to Brave's Rust adblock library. These
will be used in the upcoming qutebrowser version 2.0.0 for more granular
adblocking.

The functionality can be tested with qutebrowser 2.0.0-pre from #108272

Motivation for this change
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.

@ghost ghost requested review from FRidh and jonringer as code owners January 2, 2021 23:41
otavio
otavio suggested changes Jan 3, 2021
View reviewed changes
Copy link
Contributor

@otavio otavio left a comment •
edited

Choose a reason for hiding this comment

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

Could you also add support for the updateScript? This makes it easy to upgrade it in future.

I believe:

  passthru = {
    updateScript = nix-update-script {
      attrPath = pname;
    };
  };

Should do the trick.

@SuperSandro2000
Copy link
Member

Could you also add support for the updateScript? This makes it easy to upgrade it in future.

I believe:

  passthru = {
    updateScript = nix-update-script {
      attrPath = pname;
    };
  };

Should do the trick.

Most packages to not have this because it can be easily done with nix-update. updateScript's are usually needed for packages which are non trivial to update.

@otavio
Copy link
Contributor

otavio commented Jan 3, 2021

Could you also add support for the updateScript? This makes it easy to upgrade it in future.
I believe:

  passthru = {
    updateScript = nix-update-script {
      attrPath = pname;
    };
  };

Should do the trick.

Most packages to not have this because it can be easily done with nix-update. updateScript's are usually needed for packages which are non trivial to update.

Okay. However, I couldn't get it to work with the maintainer script without it being set. Am I missing something?

@thiagokokada
Copy link
Contributor

thiagokokada commented Jan 3, 2021 •
edited

Result of nixpkgs-review pr 108271 1

3 packages built:
  • python37Packages.adblock
  • python38Packages.adblock
  • python39Packages.adblock

Tested with #108272, seems to work fine.

@SuperSandro2000
Copy link
Member

Okay. However, I couldn't get it to work with the maintainer script without it being set. Am I missing something?

I don' think that works with nix-update. I still don't think we should add the update passthru like this for every page.

@SuperSandro2000
Copy link
Member

@ofborg eval

@ghost ghost mentioned this pull request Jan 3, 2021
Merged
10 tasks
@SuperSandro2000
Copy link
Member

Darwin patch

diff --git a/pkgs/development/python-modules/adblock/default.nix b/pkgs/development/python-modules/adblock/default.nix
index 0414ea10cba..de6d72a61ec 100644
--- a/pkgs/development/python-modules/adblock/default.nix
+++ b/pkgs/development/python-modules/adblock/default.nix
@@ -1,4 +1,6 @@
-{ rustPlatform
+{ stdenv
+, lib
+, rustPlatform
 , fetchFromGitHub
 , pipInstallHook
 , pythonImportsCheckHook
@@ -7,7 +9,8 @@
 , openssl
 , publicsuffix-list
 , isPy27
-, lib
+, CoreFoundation
+, Security
 }:
 
 rustPlatform.buildRustPackage rec {
@@ -22,12 +25,15 @@ rustPlatform.buildRustPackage rec {
     rev = version;
     sha256 = "10d6ks2fyzbizq3kb69q478idj0h86k6ygjb6wl3zq3mf65ma4zg";
   };
+  format = "pyproject";
 
   cargoSha256 = "0di05j942rrm2crpdpp9czhh65fmidyrvdp2n3pipgnagy7nchc0";
 
-  format = "pyproject";
   nativeBuildInputs = [ pipInstallHook maturin pkg-config pythonImportsCheckHook ];
-  buildInputs = [ openssl ];
+
+  buildInputs = [ openssl ]
+    ++ lib.optionals stdenv.isDarwin [ CoreFoundation Security ];
+
   PSL_PATH = "${publicsuffix-list}/share/publicsuffix/public_suffix_list.dat";
 
   buildPhase = ''
diff --git a/pkgs/top-level/python-packages.nix b/pkgs/top-level/python-packages.nix
index ea7e7853a72..105cb721a88 100644
--- a/pkgs/top-level/python-packages.nix
+++ b/pkgs/top-level/python-packages.nix
@@ -167,7 +167,9 @@ in {
 
   adb-shell = callPackage ../development/python-modules/adb-shell { };
 
-  adblock = callPackage ../development/python-modules/adblock { };
+  adblock = callPackage ../development/python-modules/adblock {
+    inherit (pkgs.darwin.apple_sdk.frameworks) CoreFoundation Security;
+  };
 
   addic7ed-cli = callPackage ../development/python-modules/addic7ed-cli { };

python3Packages.adblock: init at 0.4.0
e98eb35 
This packages python bindings to Brave's Rust adblock library. These
will be used in the upcoming qutebrowser version 2.0.0 for more granular
adblocking.
@ghost
Copy link
Author

ghost commented Jan 4, 2021

Darwin patch

Applied

@SuperSandro2000
Copy link
Member

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

3 packages built:
  • python37Packages.adblock
  • python38Packages.adblock
  • python39Packages.adblock

@ghost ghost merged commit e326297 into NixOS:master Jan 4, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SuperSandro2000 @otavio @thiagokokada