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

xsel: 1.2.0 -> git-2016-09-02 #18774

Merged
merged 1 commit into from
Sep 27, 2016
Merged

xsel: 1.2.0 -> git-2016-09-02 #18774

merged 1 commit into from
Sep 27, 2016

Conversation

cstrahan
Copy link
Contributor

Motivation for this change

The last xsel release was in 2008. Since then, a number of bugs have
been fixed. For example, pasting a moderately sized chunk of text (>4000 chars)
into chrome would cause the current tab to hang indefinitely.

A couple examples in the wild:

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@cstrahan, thanks for your PR! By analyzing the annotation information on this pull request, we identified @peti, @vcunat and @dezgeg to be potential reviewers

stdenv.mkDerivation rec {
name = "xsel-${version}";

version = "git-2016-09-02";
Copy link
Member

@rasendubi rasendubi Sep 20, 2016

Choose a reason for hiding this comment

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

git now is a part of the name. Change version to 2016-09-02-git instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point; I was just following the precedent I've seen elsewhere:

~/src/nixpkgs (nixos) λ ag 'git-\d\d\d\d-\d\d-\d\d' | wc -l
59

~/src/nixpkgs (nixos) λ ag '\d\d\d\d-\d\d-\d\d-git' | wc -l
2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just consulted doc/coding-conventions.xml, looks like what I want is "xsel-unstable-2016-09-02" as the full name, which probably means I should have name be "xsel-unstable" and version be "2016-09-02", since that's how nix would parse the full name.

If a package is not a release but a commit from a repository, then the version part of the name must be the date of that (fetched) commit. The date must be in "YYYY-MM-DD" format.
Also append "unstable" to the name - e.g., "pkgname-unstable-2014-09-23".

buildInputs = [ xlibsWrapper autoreconfHook ];

postUnpack = ''
mv $sourceRoot/README{.md,}
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rasendubi Otherwise autoconf will complain about there being no README file, and then it'll fail. I suppose I could also just touch README, or maybe there's a flag to tell autoconf to shut up...


postUnpack = ''
mv $sourceRoot/README{.md,}
'';

meta = {
Copy link
Member

Choose a reason for hiding this comment

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

Would you like to expand meta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

};

buildInputs = [xlibsWrapper];
buildInputs = [ xlibsWrapper autoreconfHook ];
Copy link
Member

Choose a reason for hiding this comment

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

If you also got rid of xlibsWrapper, it would be nice. I had only left it defined because so many packages were using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@LnL7 LnL7 added the 8.has: package (update) This PR updates a package to a newer version label Sep 20, 2016
The last xsel release was in 2008. Since then, a number of bugs have
been fixed. For example, pasting a large chunk of text (>4000 chars)
into chrome would cause the current tab to hang indefinitely.

A couple examples in the wild:

kfish/xsel#14
kfish/xsel#13
kfish/xsel#16
https://bugs.chromium.org/p/chromium/issues/detail?id=515401
electron/electron#3116
neovim/neovim#4719
Copy link
Member

@rasendubi rasendubi left a comment

Choose a reason for hiding this comment

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

Minor nitpicks. LGTM

url = http://www.vergenet.net/~conrad/software/xsel/download/xsel-1.2.0.tar.gz;
sha256 = "070lbcpw77j143jrbkh0y1v10ppn1jwmjf92800w7x42vh4cw9xr";
stdenv.mkDerivation rec {
name = "xsel-unstable-${version}";
Copy link
Member

Choose a reason for hiding this comment

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

version is not used anywhere, so could be inlined.

platforms = stdenv.lib.platforms.unix;
meta = with lib; {
description = "Command-line program for getting and setting the contents of the X selection";
homepage = "http://www.kfish.org/software/xsel";
Copy link
Member

Choose a reason for hiding this comment

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

Could be url instead of string.

@rasendubi rasendubi merged commit 4dd6d97 into NixOS:master Sep 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (update) This PR updates a package to a newer version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants