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

emacspeak: init at 51.0 #73957

Merged
merged 2 commits into from May 1, 2020
Merged

emacspeak: init at 51.0 #73957

merged 2 commits into from May 1, 2020

Conversation

@Dema
Copy link
Contributor

Dema commented Nov 23, 2019

Closes #70261

Motivation for this change

New package

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • [ x] 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 nix-review --run "nix-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.

Needs to be tested by users of emacspeak. There're a lot of irrelevant files in the sources like blog posts, speeches, radio playlists and so on. I copied what I think is needed to run it. But I don't know how to use emacspeak, so we need some real user to test if it works.

Notify maintainers

I didn't set meta.maintainers field

@adisbladis
Copy link
Member

adisbladis commented Nov 23, 2019

Thank you for your contribution!

Emacs packages should go in the emacs package set, this one should go in https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/editors/emacs-modes/manual-packages.nix.

This is not only an organisational thing but also so that functions like emacsWithPackages can pick it up.

pkgs/applications/audio/emacspeak/default.nix Outdated Show resolved Hide resolved
cd servers/native-espeak
make DESTDIR=$out install
cd ../../
Comment on lines 32 to 34

This comment has been minimized.

Copy link
@jonringer

jonringer Nov 24, 2019

Contributor

This seems odd. Do you have to call a specific Makefile?

makeFlags = [ "DESTDIR=${placeholder "out"}" ];

This comment has been minimized.

Copy link
@Dema

Dema Nov 24, 2019

Author Contributor

Yeah, there's a separate Makefile that builds tclespeak.so. Main Makefile doesn't do install. It just prints message to add current folder to emacs config. Could you explain a bit about placeholder? $out is deprecated?

This comment has been minimized.

Copy link
@jonringer

jonringer Nov 24, 2019

Contributor

in most situations, there's no difference. From what I understand, placeholder just ensures that a variable doesn't get resolved until the last possible second. I don't remember the exact scenarios in which it was necessary to add the placeholder feature tbh

This comment has been minimized.

Copy link
@jonringer

jonringer Nov 24, 2019

Contributor

but these three lines could be condensed to:

make -C servers/native-espeak DESTDIR=$out install
@Dema Dema force-pushed the Dema:emacspeak branch from 334edb3 to 0c79412 Nov 24, 2019
@Dema Dema requested a review from adisbladis as a code owner Nov 24, 2019
@Dema
Copy link
Contributor Author

Dema commented Nov 24, 2019

Moved callPackage to applications/editors/emacs-modes/manual-packages.nix but left default.nix in applications/audio, is it ok?

@adisbladis
Copy link
Member

adisbladis commented Nov 24, 2019

Please also move the expression itself.

@Dema Dema force-pushed the Dema:emacspeak branch from 0c79412 to 4eae671 Nov 24, 2019
@Dema
Copy link
Contributor Author

Dema commented Nov 24, 2019

done!

make espeak
'';
Comment on lines 26 to 27

This comment has been minimized.

Copy link
@jonringer

jonringer Nov 24, 2019

Contributor

fix alignment please

sha256 = "043mn3rr48gx2mi4rnkcvp09fwhs88yyqcszavkzvvd57mh4vd0h";
};

nativeBuildInputs = [rsync gnused] ;

This comment has been minimized.

Copy link
@jonringer

jonringer Nov 24, 2019

Contributor
Suggested change
nativeBuildInputs = [rsync gnused] ;
nativeBuildInputs = [ rsync gnused ] ;
homepage = https://github.com/tvraman/emacspeak/;
description = "Emacs extension that provides spoken output";
license = licenses.gpl2;
maintainers = [];

This comment has been minimized.

Copy link
@jonringer

jonringer Nov 24, 2019

Contributor

do you mind adding yourself as a maintainer? mostly just so you get notified if this gets bumped. Then you can verify that the software still works.

@Dema Dema force-pushed the Dema:emacspeak branch from 4eae671 to 17ad5dc Nov 25, 2019
@Dema Dema force-pushed the Dema:emacspeak branch from 17ad5dc to 043ece1 Dec 10, 2019
@Dema Dema force-pushed the Dema:emacspeak branch from 043ece1 to 375e4f0 Apr 30, 2020
@Dema
Copy link
Contributor Author

Dema commented Apr 30, 2020

Could you, please re-review PR. I got back to nixos after rage quitting it last autumn after fighting with nix language. But it's too attractive in it's core so I decided to return again :-)

nativeBuildInputs = [ rsync gnused ];
buildInputs = [ emacs tcl tclx espeak-ng ];
postPatch = ''
substituteInPlace servers/native-espeak/Makefile --replace '/usr/include/tcl$(TCL_VERSION)' ${tcl}/include

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 30, 2020

Contributor

You can overwrite both setting:

makeFlags = [ "TCL_INCLUDE=${tcl}/include" "LIBPARENTDIR = /" ];
'';

postBuild = ''
make espeak

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 30, 2020

Contributor

makeFlags won't be effective here, but make espeak just calls servers/native-espeak/Makefile. So you can also do this:

make -C servers/native-espeak "TCL_INCLUDE=${tcl}/include" "LIBPARENTDIR=/"
sha256 = "06azq2566pj4pnyawqm9387ybp3wr1pr6828hhdxhki4dpn9pj8g";
};

nativeBuildInputs = [ rsync gnused ];

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 30, 2020

Contributor

sed is already in stdenv.

Suggested change
nativeBuildInputs = [ rsync gnused ];
nativeBuildInputs = [ rsync ];
};

nativeBuildInputs = [ rsync gnused ];
buildInputs = [ emacs tcl tclx espeak-ng ];

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 30, 2020

Contributor

If emacs is used during the build for byte-compiling, it should be in nativeBuildInputs.


src = fetchFromGitHub {
owner = "tvraman";
repo = "emacspeak";

This comment has been minimized.

@Dema Dema force-pushed the Dema:emacspeak branch from 375e4f0 to f78eabe Apr 30, 2020
@Dema
Copy link
Contributor Author

Dema commented Apr 30, 2020

Updated as per suggestions and took installation code from arch package :), looks a bit cleaner.

Closes #70261
@Dema Dema force-pushed the Dema:emacspeak branch from 09aa9df to e43ad6e Apr 30, 2020
@Dema
Copy link
Contributor Author

Dema commented Apr 30, 2020

Forgot to push even more clean version :)

@Dema Dema changed the title emacspeak: init at 50.0 emacspeak: init at 51.0 Apr 30, 2020
This usually causes less problems w.r.t. hard-codes paths.
@Mic92
Copy link
Contributor

Mic92 commented May 1, 2020

This was working for me now:

$ nix-shell -p emacsPackages.emacspeak --run "emacspeak"

I heard espeak reading out emacs.

@Mic92 Mic92 merged commit 170f301 into NixOS:master May 1, 2020
1 check was pending
1 check was pending
grahamcofborg-eval Checking new out paths
Details
@Dema Dema deleted the Dema:emacspeak branch May 2, 2020
@adisbladis
Copy link
Member

adisbladis commented May 12, 2020

The meta attribute in this derivation was wrong, fixed in a2ad2ba.

Also @Dema it seems like you are not in the maintainers list. If you want to maintain packages in nixpkgs please add yourself to the maintainers list & re-add yourself to the derivation in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.