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

dstask: init at 0.18 #87383

Merged
merged 2 commits into from May 30, 2020
Merged

dstask: init at 0.18 #87383

merged 2 commits into from May 30, 2020

Conversation

stianlagstad
Copy link
Contributor

@stianlagstad stianlagstad commented May 9, 2020

Motivation for this change

I'd like to still use https://github.com/naggie/dstask after switching from Ubuntu to NixOS.

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.

@wamserma
Copy link
Member

wamserma commented May 9, 2020

undefined variable 'stianlagstad' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/packet-spot-eval-1/pkgs/applications/misc/dstask/default.nix:24:39

Please also add yourself to https://github.com/NixOS/nixpkgs/blob/master/maintainers/maintainer-list.nix

@symphorien
Copy link
Member

Please consider building from source instead. Go packages should be rather easy to package, see https://nixos.org/nixpkgs/manual/#sec-language-go

@stianlagstad
Copy link
Contributor Author

@symphorien Thanks for responding. I'm giving it a shot. Here's my current attempt:

{ stdenv, buildGoModule, fetchFromGitHub }:

buildGoModule rec {
  pname = "dstask";
  version = "0.17";

  src = fetchFromGitHub {
    owner = "naggie";
    repo = "dstask";
    rev = "{$version}";
    sha256 = "1z00f7ar8a0ahspgc240iq7cka1anls383d1fv3i17sb6ajmp6rc"; # I got this from `nix-prefetch-url --unpack https://github.com/naggie/dstask/archive/47cb278daccc520d73a9838462d60f514aedd87f.tar.gz`
  };

  modSha256 = "1z00f7ar8a0ahspgc240iq7cka1anls383d1fv3i17sb6ajmp6rc"; # I have no idea where to get this from. For now just copied in the same as the above.

  subPackages = [ "." ];

  meta = with stdenv.lib; {
    description = "Command line todo list with super-reliable git sync";
    homepage = "https://github.com/naggie/dstask";
    license = licenses.mit;
    maintainers = with maintainers; [ stianlagstad ];
    platforms = platforms.linux;
  };
}

When running nix-build -A dstask -K (from the root directory) I get this:

> nix-build -A dstask -K
these derivations will be built:
  /nix/store/vpayn6fjjc03zyly1dkm1mai09vbzhs0-source.drv
  /nix/store/nxmv80pajayd7p8qdkah0zmqsz7dnzkq-dstask-0.17-go-modules.drv
  /nix/store/1mdr6071k78krbahy0hqd0lnzh1amnjh-dstask-0.17.drv
building '/nix/store/vpayn6fjjc03zyly1dkm1mai09vbzhs0-source.drv'...

trying https://github.com/naggie/dstask/archive/{$version}.tar.gz
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   125  100   125    0     0    291      0 --:--:-- --:--:-- --:--:--   291
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
curl: (22) The requested URL returned error: 404 Not Found
error: cannot download source from any mirror
note: keeping build directory '/tmp/nix-build-source.drv-6'
builder for '/nix/store/vpayn6fjjc03zyly1dkm1mai09vbzhs0-source.drv' failed with exit code 1
cannot build derivation '/nix/store/1mdr6071k78krbahy0hqd0lnzh1amnjh-dstask-0.17.drv': 1 dependencies couldn't be built
error: build of '/nix/store/1mdr6071k78krbahy0hqd0lnzh1amnjh-dstask-0.17.drv' failed

I don't understand why it's trying to fetch "https://github.com/naggie/dstask/archive/{$version}.tar.gz" instead of "https://github.com/naggie/dstask/archive/0.17.tar.gz". Could you give me a pointer?

@symphorien
Copy link
Member

I think you meant "${version}"; instead of "{$version}";

About hashes, replace them with zeroes, and nix will give you the right hash in the error message.

@stianlagstad
Copy link
Contributor Author

Thanks! I now tried this:

{ stdenv, buildGoModule, fetchFromGitHub }:

buildGoModule rec {
  pname = "dstask";
  version = "0.17";

  src = fetchFromGitHub {
    owner = "naggie";
    repo = "dstask";
    rev = "${version}";
    sha256 = "1z00f7ar8a0ahspgc240iq7cka1anls383d1fv3i17sb6ajmp6rc";
  };

  modSha256 = "1ydzqg8p2d514sdb34b2p6k1474nr1drrn3gay2cpyhrj5l51cj3";

  subPackages = [ "." ];

  meta = with stdenv.lib; {
    description = "Command line todo list with super-reliable git sync";
    homepage = "https://github.com/naggie/dstask";
    license = licenses.mit;
    maintainers = with maintainers; [ stianlagstad ];
    platforms = platforms.linux;
  };
}

Here's what happen when I build and look in the result directory:

> nix-build -A dstask -K                                         
these derivations will be built:
  /nix/store/kjg11z4j8cspzg03jg52bdxk0nxhghr9-dstask-0.17.drv
building '/nix/store/kjg11z4j8cspzg03jg52bdxk0nxhghr9-dstask-0.17.drv'...
unpacking sources
unpacking source archive /nix/store/9kxb3dig60vxwf52gdz498kbh7b5w6n8-source
source root is source
patching sources
configuring
building
Building subPackage ./.
go: downloading golang.org/x/sys v0.0.0-20181213200352-4d1cda033e06
go: downloading gopkg.in/yaml.v2 v2.2.2
go: downloading github.com/gofrs/uuid v3.2.0+incompatible
runtime/cgo
net
github.com/gofrs/uuid
golang.org/x/sys/unix
gopkg.in/yaml.v2
os/user
github.com/naggie/dstask
installing
post-installation fixup
find: '/nix/store/599xncfk2ayd40v2v3ns851nv1jrv27c-dstask-0.17/bin': No such file or directory
shrinking RPATHs of ELF executables and libraries in /nix/store/599xncfk2ayd40v2v3ns851nv1jrv27c-dstask-0.17
strip is /nix/store/3b3ighb83nhifa1v4n7855hlbdl1mhf9-binutils-2.31.1/bin/strip
patching script interpreter paths in /nix/store/599xncfk2ayd40v2v3ns851nv1jrv27c-dstask-0.17
checking for references to /build/ in /nix/store/599xncfk2ayd40v2v3ns851nv1jrv27c-dstask-0.17...
/nix/store/599xncfk2ayd40v2v3ns851nv1jrv27c-dstask-0.17
> ls -ahl /nix/store/599xncfk2ayd40v2v3ns851nv1jrv27c-dstask-0.17
total 980K
dr-xr-xr-x    2 root root   4.0K Jan  1  1970 .
drwxrwxr-t 2190 root nixbld 972K May  9 19:55 ..

Note the find: '/nix/store/599xncfk2ayd40v2v3ns851nv1jrv27c-dstask-0.17/bin': No such file or directory there. Does that mean that the build of https://github.com/naggie/dstask/ outputs the binary in another folder or something?

@symphorien
Copy link
Member

Honestly, I'm not very familiar with go. Maybe we can wait for someone more knowledgeable or you can ask on irc or on discourse ?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/help-building-a-go-package/7117/1

@stianlagstad
Copy link
Contributor Author

Thanks again @symphorien . I've asked for help on the nixos discourse.

@stianlagstad
Copy link
Contributor Author

Thanks to foxit64 here the build now works!

@stianlagstad stianlagstad changed the title dstask: init at 0.17 dstask: init at 0.18 May 21, 2020
@stianlagstad
Copy link
Contributor Author

The build error says:

modSha256 is deprecated and will be removed in the next release (20.09), use vendorSha256 instead

I tried building locally just using vendorSha256 instead of modSha256 but that gave the error:

error: anonymous function at /nix/var/nix/profiles/per-user/root/channels/nixos/pkgs/development/go-modules/generic/default.nix:3:1 called without required argument 'modSha256', at /home/stian/devp/nixpkgs/pkgs/applications/misc/dstask/default.nix:3:1

Does that mean that buildGoModule itself need to be changed for the build to pass?

@foxit64
Copy link
Contributor

foxit64 commented May 21, 2020

@stianlagstad
Try it. I'm seeing this build error for the first time.

@ofborg ofborg bot requested review from kalbasit and foxit64 May 23, 2020 20:14
@stianlagstad
Copy link
Contributor Author

Looking at go-modules/generic/default.nix I saw this recent change. I've updated the PR to use vendorSha256 instead of modSha256.

@stianlagstad
Copy link
Contributor Author

I see that these two checks fail with:

vendor folder exists, please set 'vendorSha256=null;' or 'deleteVendor=true;' in your expression

That's from this part of the nix expression:

    buildPhase = args.modBuildPhase or ''
      runHook preBuild
      if [ ${deleteFlag} == "true" ]; then
        rm -rf vendor
      fi
      if [ -e vendor ]; then
        echo "vendor folder exists, please set 'vendorSha256=null;' or 'deleteVendor=true;' in your expression"
        exit 10
      fi
      go mod vendor
      mkdir -p vendor
      runHook postBuild
    '';

I don't understand enough of this yet to know what to do. @naggie are you able to assist?

@naggie
Copy link

naggie commented May 24, 2020

I think vendorSha256=null should do it, as I do vendor my go modules (which means storing the thirdparty code in the repository)

I vendor the modules to make sure it's possible to build dstask even if the thirdparty dependencies are removed/changed from github.

@naggie
Copy link

naggie commented May 24, 2020

Also I wonder if we need to add -mod=vendor to the buildFlagsArray

@stianlagstad
Copy link
Contributor Author

stianlagstad commented May 24, 2020

@naggie Thanks! Looks like I messed up that build flag in my change though. The build log of the new failure:

flag provided but not defined: -mod

(from https://logs.nix.ci/?key=nixos/nixpkgs.87383&attempt_id=617729ef-59b3-437b-af72-45c4176daa3f)

@naggie
Copy link

naggie commented May 24, 2020

I think that needs its own new line, else it's probably parsed as an ldflag:

 buildFlagsArray = [ ''-ldflags=-w -s
    -mod=vendor
    -X "github.com/naggie/dstask.VERSION=${version}"
    -X "github.com/naggie/dstask.GIT_COMMIT=${rev}"
  '' ];

(a guess -- I've not had time to get nix/nixos running)

Copy link
Contributor

@foxit64 foxit64 left a comment

Choose a reason for hiding this comment

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

LGTM

@stianlagstad
Copy link
Contributor Author

Thanks again @naggie and @foxit64 .

@foxit64 : Do you know if there's anything else that needs to be done to move the PR forward? Or do I just need to be a little more patient? :)

@ofborg ofborg bot requested a review from foxit64 May 27, 2020 15:58
Copy link
Contributor

@foxit64 foxit64 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/172

@Lassulus
Copy link
Member

squashing/rebasing the commits into 2 would be nice:

  • one for the maintainer addition
  • one for the package

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Sorry for holding this up even longer, but there are a few things I would like to see clarified. Its important to document everything as well as possible in the expression itself, since the next person looking at it will not have the context you have right now (even if the next person is you).

I think we're close to the finish line though. Once you have made all the changes, please squash your commits so that only two remain:

maintainers: add stianlagstad
dstask: init at 0.18

(in that order). Note that the format of the maintainers commit message needs to be amended and the version in the init commit needs to be changed to 0.18.

If you're not familiar with the workflow, have a look at https://about.gitlab.com/blog/2018/06/07/keeping-git-commit-history-clean/ (situation 3).

Looking forward to merging this :)

pkgs/applications/misc/dstask/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/dstask/default.nix Show resolved Hide resolved
@stianlagstad
Copy link
Contributor Author

Thank you @Lassulus and @timokau ! I'll have to wait for help from either @foxit64 or @naggie to explain those build flags. I'll rewrite the commits as required after adding a comment explaining the build flags.

@ofborg ofborg bot requested a review from foxit64 May 28, 2020 19:32
@naggie
Copy link

naggie commented May 28, 2020

Sorry for holding this up even longer,

No problem, I'm glad the standards are so high

@stianlagstad
Copy link
Contributor Author

stianlagstad commented May 29, 2020

I've updated the commits now. Thank you all for the help so far, and please do feel free to comment further as I'm in no particular rush and would like to follow the standards.

Comment on lines 28 to 29
-X "github.com/naggie/dstask.VERSION=v${version}"
-X "github.com/naggie/dstask.GIT_COMMIT=${rev}"
Copy link
Member

Choose a reason for hiding this comment

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

How about this:

Suggested change
-X "github.com/naggie/dstask.VERSION=v${version}"
-X "github.com/naggie/dstask.GIT_COMMIT=${rev}"
-X "github.com/naggie/dstask.VERSION=${version}"
-X "github.com/naggie/dstask.GIT_COMMIT=v${version}"

I don't see why there should be a v prefix for the version. At the same time, v${version} is the name of the tag and therefore a valid specifier of a git commit.

Copy link

@naggie naggie May 30, 2020

Choose a reason for hiding this comment

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

Seems like a good change as it would keep the output of dstask version consistent with the official binary, which currently outputs:

Version: v0.18
Git commit: d687a0f9aec634a808cfa0df4aecb06ab4f57732
Build date: Wed 13 May 18:34:38 BST 2020

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both again! I've updated the PR.

Copy link
Member

Choose a reason for hiding this comment

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

@naggie it sound like you may be reading the diff the wrong way around. Previously it would have output

Version: v0.18
Git commit: d687a0f9aec634a808cfa0df4aecb06ab4f57732

but now it should be

Version: 0.18
Git commit: v0.18

The reasoning behind this is that we would like to avoid having to manually set the fully-qualified git revision of the release in our expression. That would be cumbersome and our automated tooling wouldn't update that. At the same time, since the version is tagged v0.18 is also a valid reference to the git revision.

We've dropped the v prefix from Version since 0.18 is the version number while v0.18 is the name of the tag.

Is that fine by you?

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Looks good to me now, except that one line and pending approval of @naggie regarding the version/rev setting.

pkgs/applications/misc/dstask/default.nix Outdated Show resolved Hide resolved
Thank you to: foxit64, naggie, timokau, Lassulus, 06kellyjac,
symphorien, wamserma.
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Looks great to me now. I'll merge this if and when @naggie approves. If I forget about this, please ping me again some time next week.

@timokau
Copy link
Member

timokau commented May 30, 2020

Thanks everyone for your patience and responsiveness. I think the end-result is a high-quality package. I'll probably check it out some time, I have even thought about working on a taskwarrior alternative myself already.

Welcome to the team @stianlagstad 🎉

@timokau timokau merged commit 69ea180 into NixOS:master May 30, 2020
@naggie
Copy link

naggie commented May 30, 2020

Great, thanks to everyone, really appreciated!

I'm new to nix, I assume it will end up in nix-unstable after the build server grabs it?

@timokau
Copy link
Member

timokau commented May 30, 2020

Pretty much. The build server periodically grabs the latest nixpkgs revision and builds all packages. Once that is done and some checks pass, the nixos-unstable channel is updated to that new revision. It'll probably take something between a day and a week until the package shows up in the channel.

You don't have to wait for that though. Nix makes it possible to build packages or even your whole system from any nixpkgs checkout, it doesn't have to be a channel. The packages that have already been built will transparently be pulled in from the binary caches, everything else will be built from source. Imagine a blend of ArchLinux and Gentoo. The point of channels is mostly to signal everyone "the binary channel for this revision is populated and the tests have passed".

@naggie
Copy link

naggie commented May 30, 2020

Oh I see, I like the sound of that. I prefer this to the AUR due to the review process, and the fact there's the binary cache means things are fast where possible.

@foxit64
Copy link
Contributor

foxit64 commented May 30, 2020

Congratulations to @stianlagstad for your first successful merge! Thanks to everyone!

@stianlagstad stianlagstad deleted the sl/dstask-0.17 branch May 30, 2020 15:20
@stianlagstad
Copy link
Contributor Author

stianlagstad commented May 31, 2020

Thank you everyone for the help!

In case anyone else stumbles upon this and wants to install dstask before its available in a channel (i.e. me a little while ago): See Adding Custom Packages in the NixOS manual.

@naggie
Copy link

naggie commented Jun 4, 2020

Cool, it's now listed: https://nixos.org/nixos/packages.html?channel=nixpkgs-unstable&query=dstask

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

Successfully merging this pull request may close these issues.

None yet

10 participants