-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
bleachbit: 2.0 -> 2.2 #58152
bleachbit: 2.0 -> 2.2 #58152
Conversation
chmod +x $out/bin/bleachbit | ||
substituteInPlace $out/bin/bleachbit --replace "#!/usr/bin/env python" "#!${pythonPackages.python.interpreter}" | ||
make install SHELL=${stdenv.shell} prefix=${placeholder "out"} |
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.
Can't you drop this entirely and just use makeFlags
?
On Fri, 22 Mar 2019 19:08:20 -0700, worldofpeace ***@***.***> wrote:
worldofpeace commented on this pull request.
> };
+ nativeBuildInputs = [ gettext ];
buildInputs = [ pythonPackages.wrapPython ];
```suggestion
```
Unneeded as it's done in `mk-python-derivation.nix`.
Hmm, curious. If I remove it the build fails trying to run `msgfmt`.
Or does it work for you w/o the `gettext` input?
…
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#58152 (review) part: text/html
|
On Fri, 22 Mar 2019 19:09:29 -0700, worldofpeace ***@***.***> wrote:
worldofpeace commented on this pull request.
> doCheck = false;
postInstall = ''
- mkdir -p $out/bin
- cp bleachbit.py $out/bin/bleachbit
- chmod +x $out/bin/bleachbit
-
- substituteInPlace $out/bin/bleachbit --replace "#!/usr/bin/env python" "#!${pythonPackages.python.interpreter}"
+ make install SHELL=${stdenv.shell} prefix=${placeholder "out"}
Can't you drop this entirely and just use `makeFlags`?
Not AFAICT? Normally yes (and wouldn't need to set SHELL ourselves) but
I don't see a way to do that here.
If that seems wrong or it works for you LMK haha :).
…
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#58152 (review) part: text/html
|
@dtzWill I think my comment on in email appears to be different than what I communicated. For example I suggested to remove |
Per reviewer feedback, thanks!
Ahh realized that we should set diff --git a/pkgs/applications/misc/bleachbit/default.nix b/pkgs/applications/misc/bleachbit/default.nix
index b2348bbf752..7b4b9cd0885 100644
--- a/pkgs/applications/misc/bleachbit/default.nix
+++ b/pkgs/applications/misc/bleachbit/default.nix
@@ -3,6 +3,8 @@ pythonPackages.buildPythonApplication rec {
pname = "bleachbit";
version = "2.2";
+ format = "other";
+
src = fetchurl {
url = "mirror://sourceforge/${pname}/${pname}-${version}.tar.bz2";
sha256 = "1yj9bc3k6s1aib7znb79h5rybfv691zz4szxkwf9fm9nr0dws603";
@@ -17,11 +19,13 @@ pythonPackages.buildPythonApplication rec {
find -type f -exec sed -i -e 's@/usr/bin@${placeholder "out"}/bin@g' {} \;
'';
- doCheck = false;
+ dontBuild = true;
- postInstall = ''
- make install SHELL=${stdenv.shell} prefix=${placeholder "out"}
- '';
+ installFlags = [ "prefix=${placeholder "out"}" ];
+
+ doCheck = true;
+
+ checkTarget = "tests";
propagatedBuildInputs = with pythonPackages; [ pygtk ];
|
On Fri, 22 Mar 2019 21:42:55 -0700, worldofpeace ***@***.***> wrote:
@dtzWill I think my comment on the mailing list appears to be different than what I communicated.
For example I suggested to remove `wrapPython`not `gettext`. and said
"Unneeded as it's done in mk-python-derivation.nix.".
...
Yes you are correct, I completely misread/misunderstood.
Sorry :3. Pushed commit making the change, good call thanks! :)
…
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#58152 (comment) part: text/html
|
Patch by @worldbypeace, during PR review. Thanks!
Don't see the tests running in the output, not sure why. Guess we should just leave that out so it doesn't look confusing. |
ping @dtzWill |
Pushed commit disabling tests, sorry for delay.
Was hoping to fix them (moving to `installCheckPhase = ` gets them
running) but too many small details to fixup so punting on that for now.
…On Sun, 31 Mar 2019 21:56:31 -0700, worldofpeace ***@***.***> wrote:
ping @dtzWill
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#58152 (comment) part: text/html
|
Looks good. It just needs a squash. I'd try to maybe keep some of the comments on what was done. Other than that this works 👍 . |
Motivation for this change
https://www.bleachbit.org/news/bleachbit-22
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)