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
twinkle: init at 1.10.2 #74767
twinkle: init at 1.10.2 #74767
Conversation
Could you please follow the commit message convention? |
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.
Also the separate commit to add yourself to maintainers-list should read
maintainers: add mkg20001
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
367c441
to
5f98e27
Compare
Can this get merged please? |
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.
Looks good now.
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
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.
When you think the PR is mergeable, please squash the commit history into two commits (one adding yourself as a maintainer, one adding the package).
There is no need to maintain commit history within a PR. Commits should be logical units that could be useful in the future, e.g. for a revert, a bisect, a cherry-pick. For example it might make sense to revert the package addition without also reverting your addition to the maintainers list. But there is no reason to keep fixup commits like "twinkle: fix description".
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
|
||
nativeBuildInputs = [ cmake bison flex wrapQtAppsHook bcg729 ]; | ||
|
||
cmakeFlags = [ "-DWITH_G729=On" "-DWITH_SPEEX=On" "-DWITH_ILBC=On" /* "-DWITH_DIAMONDCARD=On" broken code */ ]; |
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.
What does "broken code" mean?
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.
Seems ancient and broken
/build/source/src/gui/mphoneform.cpp: In member function 'void MphoneForm::init()':
/build/source/src/gui/mphoneform.cpp:244:9: error: 'class QMenu' has no member named 'insertItem'; did you mean 'insertMenu'?
menu->insertItem("Diamondcard", Diamondcard);
^~~~~~~~~~
insertMenu
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
5238df3
to
7eb798f
Compare
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
e6ea734
to
eb71e9b
Compare
|
||
cmakeFlags = [ "-DWITH_G729=On" "-DWITH_SPEEX=On" "-DWITH_ILBC=On" /* "-DWITH_DIAMONDCARD=On" seems ancient and broken */ ]; | ||
|
||
enableParallelBuilding = true; |
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 is default 031138b, no need to specify
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.
But only for cmake and not meson?
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.
Ah, the commit I mentioned is for cmake which this project uses. For meson it's for sure a default.
pkgs/applications/networking/instant-messengers/twinkle/default.nix
Outdated
Show resolved
Hide resolved
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.
LGTM. Could you add a changelog meta attribute?
changelog = "https://github.com/LubosD/twinkle/blob/${version}/NEWS";
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.
Thanks!
So, somewhere in between I messed up and pushed an older version Fixed that |
Motivation for this change
Need this for myself, useful for others as well.
Things done
Added the twinkle package
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers