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

Gogs: init at 20160304-d57a2b9 #13566

Closed
wants to merge 3 commits into from
Closed

Gogs: init at 20160304-d57a2b9 #13566

wants to merge 3 commits into from

Conversation

valeriangalliat
Copy link
Contributor

The default.nix is generated using go2nix and applying a couple of patches to make the SQLite support configurable and wrapping the program.

Gogs behavior is a bit modified by the wrapper program because original Gogs is impure (it expects to be run only in its source Git repository and needs to be able to write files inside. To avoid this, I define the GOGS_WORK_DIR to be the CWD if not defined, and cd inside this directory before executing Gogs (if Gogs is not run from inside the GOGS_WORK_DIR, some parts still rely on the CWD to be the work directory).

Also the public and templates directories from the Go package directory are symlinked inside the work directory since Gogs expect them to be here.

This PR comes with a maintenance script, update-gogs-definition, that is basically doing an impure go get of chosen Gogs revision and its dependencies, and runs go2nix over this tree (plus some specific patches).

For information, I also have a WIP NixOS module integration for Gogs but I'll add it later, or in another PR, once I'm done with it.

Dependencies

I'm not sure about the way dependencies are handled here. It's plain go2nix but it's not making those dependencies shareable... but it would be a huge work to go through those 46 dependencies, and convert them in nixpkgs.goPackages format, see if they exist already and in a compatible version or add the definition, and requiring to manually recreate the dependency graph.

I'd really like some input about the manner to handle this.

Things done:
  • Tested using sandboxing (nix-build --option build-use-chroot true or nix.useChroot on NixOS)
  • Built on platform(s): NixOS
    • Builds on OSX but the update-gogs-definition script does not work because go get on OSX in unable to verify the certificates of gopkg.in and I couldn't find a way to fix this.
  • Tested execution of all binary files (usually in ./result/bin/)

postInstall = ''
wrapProgram $bin/bin/gogs \
--prefix PATH : ${git}/bin \
--run 'export GOGS_WORK_DIR=${"$" + "{"}GOGS_WORK_DIR:-$PWD}' \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line looks odd, because I didn't find any way to escape ${ inside a Nix string.

The documentation says to put a \ in front of it but this did not work for me, still yielding unexpected $undefined error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use the nix string open / close symbol to escape, ex ''${}
On Sun, Feb 28, 2016 at 8:25 PM Valérian Galliat notifications@github.com
wrote:

In pkgs/applications/version-management/gogs/default.nix
#13566 (comment):

+with goPackages;
+
+buildGoPackage rec {

  • name = "gogs-${version}";
  • version = "20160227-${stdenv.lib.strings.substring 0 7 rev}";
  • rev = "d320915ad2a7b4bbab075b98890aa50f91f0ced5";
  • buildInputs = [ makeWrapper go ];
  • buildFlags = lib.optional (sqliteSupport) "-tags sqlite";
  • goPackagePath = "github.com/gogits/gogs";
  • postInstall = ''
  • wrapProgram $bin/bin/gogs \
  •  --prefix PATH : ${git}/bin \
    
  •  --run 'export GOGS_WORK_DIR=${"$" + "{"}GOGS_WORK_DIR:-$PWD}' \
    

This line looks odd, because I didn't find any way to escape ${ inside a
Nix string.

The documentation http://nixos.org/nix/manual/#idm46912467695696 says
to put a \ in front of it but this did not work for me, still yielding unexpected
$undefined error.


Reply to this email directly or view it on GitHub
https://github.com/NixOS/nixpkgs/pull/13566/files#r54368220.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@jagajaga
Copy link
Member

Please properly name commit to follow our rules in CONTRIBUTING.md.

@valeriangalliat valeriangalliat changed the title Add Gogs package Gogs: add derivation Feb 29, 2016
@valeriangalliat
Copy link
Contributor Author

@jagajaga Thanks, updated.

@jgillich
Copy link
Member

I'm not too familiar with the go stuff, but why isn't this part of goPackages?

Also, the correct commit message would be gogs: init at 20160227

@valeriangalliat valeriangalliat changed the title Gogs: add derivation Gogs: init at 20160227-d320915 Feb 29, 2016
@valeriangalliat
Copy link
Contributor Author

@jgillich Oh sorry I didn't get it for the commit message. Updated.

About the goPackages, that's the doubt/question I have in the Dependencies part of my PR message; I used go2nix to generate this derivation but it makes a single derivation for the Gogs package and all its dependencies at an exact version merged together, instead of a shared dependencies tree like in goPackages. I'm aware of no tooling to do this yet, and doing it manually would take much time, both now and eveytime we update Gogs (compared to the current fully automated update-gogs-definition script).

@joachifm
Copy link
Contributor

joachifm commented Mar 5, 2016

Looks like a legit build failure:

# github.com/gogits/gogs/models
go/src/github.com/gogits/gogs/models/repo.go:374: cannot use true (type bool) as type git.PullRemoteOptions in argument to git.Pull
builder for ‘/nix/store/7bbw82k9zd4fibzgz7g9z22dy40ilxl1-go1.5-gogs-20160227-d320915.drv’ failed with exit code 44
error: build of ‘/nix/store/7bbw82k9zd4fibzgz7g9z22dy40ilxl1-go1.5-gogs-20160227-d320915.drv’ failed
The invocation of "nix-build -A gogs /home/travis/.nox/nixpkgs" failed

@valeriangalliat valeriangalliat changed the title Gogs: init at 20160227-d320915 Gogs: init at 20160304-d57a2b9 Mar 5, 2016
@valeriangalliat
Copy link
Contributor Author

Oops right, thanks, related to gogs/gogs#2728, I just updated Gogs and its dependencies.

@@ -0,0 +1,22 @@
2,3c2
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this exists? Patching default.nix is a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joachifm default.nix is generated by go2nix, but it cannot guess the postInstall hook required to wrap Gogs (because it's specific to Gogs), nor the optional SQLite support with an argument.

That's why, when auto-generating default.nix using gogs-derivation.nix, I patch the output of go2nix to include the sqliteSupport argument, and postInstall hook, which makes the final default.nix.

All of this is done inside nix-build --no-out-link gogs-definition.nix.

Maybe I could achieve the same by having the raw output of go2nix in default-generated.nix, and overriding it in default.nix to add sqliteSupport and postInstall?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit complex for my tastes, but I'm simply not qualified to comment on go packaging, sorry.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I immediately don't see any better way than this.

I also didn't know about go2nix, so perhaps its users might have a better idea how to handle such a situation. Maybe @cstrahan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot @cstrahan, I think it's possible to override the go2nix output to add the postInstall hook, and tweak the buildFlags, it's indeed a lot cleaner than patching default.nix.

However the conditional sqliteSupport is a bit trickier, since it needs to remove an entry from extraSrcs if sqliteSupport is false, and extraSrcs is an argument to buildGoPackage, but not an attribute of the actual derivation, so I think that lib.overrideDerivation will not help. I can just ignore this part, it will result in downloading an unnecessary Go package if sqliteSupport is false which is not that bad.

But if anyone have an idea how to gracefully handle this with overriding, I would happily take it. 😄

Copy link
Member

Choose a reason for hiding this comment

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

I agree that generated defaults should be easily overridable. I'll look at haskell stuff and try to make it better.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to separate default.nix from generated one in kamilchm/go2nix#10 but it looks like the allowGoReference flag doesn't work :/
Is there a better way to override generated file than import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kamilchm Oh, nice! But yes allowGoReference is an argument of buildGoPackage and lib.overrideDerivation allows only to custom the derivation attributes.

Is there some kind of override mechanism in Nix that would allow to customize the arguments to the buildGoPackage call?

An idea would be to tweak the generated.nix like this:

{ stdenv, lib, goPackages, fetchgit, fetchhg, fetchbzr, fetchsvn, args }:

with goPackages;

buildGoPackage (rec {
  name = "go2nix-${version}";
  # ...
} // args)

And call it like this:

callPackage ./generated.nix {
  args = {
    allowGoReference = true;
    # ...
  };
}

Or even allow to pass these arguments in the top-level function instead of the args set, like @cstrahan suggested, if I understood well? Something like the following example would avoid listing explicitly all possible overridable attributes, but I don't like this filterAttrs call, maybe there's a better way to get all the remaining args).

{ stdenv, lib, goPackages, fetchgit, fetchhg, fetchbzr, fetchsvn, ... } @ args:

with goPackages;

buildGoPackage (rec {
  name = "go2nix-${version}";
  # ...
} // (lib.filterAttrs (n: v: n != "lib" && n != "stdenv" && n != "goPackages" && n != "fetchbzr" && n != "fetchgit" && n != "fetchsvn" && n != "fetchhg") args))

Copy link
Member

Choose a reason for hiding this comment

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

@cstrahan what do you think about these options?

@valeriangalliat
Copy link
Contributor Author

Quick update of gogs-derivation.nix to remove usage of my fork of go2nix and just use goPackages.go2nix since #13741, #13564, and #13533 are merged.

@kamilchm
Copy link
Member

@valeriangalliat see new go2nix from master and how i rework goPackages in #16017

@garbas
Copy link
Member

garbas commented Jul 23, 2016

@valeriangalliat ping

@valeriangalliat
Copy link
Contributor Author

Thanks a lot @kamilchm, I updated the PR to use new go2nix generated default.nix and deps.json.

I can't build it though:

stripping (with flags -S) in /nix/store/wca3v40cfbrman7xxnmaww7i925ha6qy-go1.6-gogs-20160728-ad7ea88-bin/bin
strip: can't process non-object and non-archive file: /nix/store/wca3v40cfbrman7xxnmaww7i925ha6qy-go1.6-gogs-20160728-ad7ea88-bin/bin/gogs
patching script interpreter paths in /nix/store/wca3v40cfbrman7xxnmaww7i925ha6qy-go1.6-gogs-20160728-ad7ea88-bin
cycle detected in the references of ‘/nix/store/3kqbi3as85hshj1kxifhqsdmlprqnad7-go1.6-gogs-20160728-ad7ea88’
error: build of ‘/nix/store/hhr8wdwfyxiz2ccwny9jbalg4gqwwvxw-go1.6-gogs-20160728-ad7ea88.drv’ failed

Maybe something doesn't like the fact that because of wrapProgram, the Gogs wrapper shell script references the Gogs package to actually access the binary?

@kamilchm
Copy link
Member

What about last line with ln in postInstall. It looks like it could generate a cycle?

@valeriangalliat
Copy link
Contributor Author

I removed whole postInstall and still have the same problem, not sure how to investigate on this.

grep -R /nix/store/ipgqsf1myv4fk5dd9r5pv0lgpkf5kps9-go1.6-gogs-20160728-ad7ea88 /nix/store/zmama6l4ai64adzix0bfc8lwkc81bnz9-go1.6-gogs-20160728-ad7ea88-bin
Binary file /nix/store/zmama6l4ai64adzix0bfc8lwkc81bnz9-go1.6-gogs-20160728-ad7ea88-bin/bin/gogs matches
Binary file /nix/store/zmama6l4ai64adzix0bfc8lwkc81bnz9-go1.6-gogs-20160728-ad7ea88-bin/bin/gogs-ad7ea88 matches

Not sure if this is OK or if this is the problem?

@kamilchm
Copy link
Member

kamilchm commented Jul 29, 2016

I think it's related to NixOS/nix#494
It could be a lot easier to debug with NixOS/nix#925
It builds fine for me when I remove the ln so I suspect that you can't do wrapProgram in $bin output when the wrapper needs anything from $out output.
I would try to install data files into a separate output and link it then from $bin

@kamilchm
Copy link
Member

kamilchm commented Jul 29, 2016

It works fine when I do kamilchm@c491474
But I wonder if there could be a better way to use templates and public than linking it in PWD?
Maybe we should use go-bindata to embed static files into the binary? https://github.com/gogits/gogs/blob/master/Makefile#L46

PS. Don't forget to add metadata https://nixos.org/nixpkgs/manual/#sec-standard-meta-attributes ;)

@valeriangalliat
Copy link
Contributor Author

Oh thanks, I'll use this then, I'll update the PR this week-end.

Other than modifying Gogs source code, last time I checked, I could think of any other way than linking templates in GOGS_WORK_DIR and also running the program from GOGS_WORK_DIR.

Because Gogs expects to be run from its Go repository, they import / read some paths relative to CWD, and some others from GOGS_WORK_DIR.

@yorickvP
Copy link
Contributor

@valeriangalliat what's the status of this?

@valeriangalliat
Copy link
Contributor Author

Forgot about it, sorry, just pulled @kamilchm patch, hopefully the tests will pass.

@yorickvP
Copy link
Contributor

yorickvP commented Sep 12, 2016

Don't pay too much attention to the tests #6652
But do pay some attention, it seems like it has something to do with it this time.

@domenkozar
Copy link
Member

In this care the error on OSX is legit.

@schneefux schneefux mentioned this pull request Oct 8, 2016
6 tasks
@Mic92
Copy link
Member

Mic92 commented Oct 22, 2016

This pull request is superseded by #19361

@fpletz
Copy link
Member

fpletz commented Nov 28, 2016

Closing in favor of #19361. Thanks for your contribution!

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