On 2016-12-16 02:52, Leo Famulari wrote: > On Thu, Dec 15, 2016 at 10:06:59PM +0100, Petter wrote: >> Hi again, >> >> I've prefixed most of the packages with "golang-" now. However, there >> are >> some >> packages already starting with "golang-", f.ex. >> "golang-org-x-text-unicode-norm", >> and I left those alone. It's inconsistent, but I felt this was >> preferable to >> names like "golang-golang-org-x-text-unicode-norm". What do you think? > > That's the right way. It fits the package naming guidelines: > > https://www.gnu.org/software/guix/manual/html_node/Package-Naming.html I'm able to interpret your answer both ways, should I make it "golang-golang-org-x-text-unicode-norm"? >> Finally, there's a telemetry configuration in Syncthing. It is opt-in; >> but it will ask the user after a few hours if they want to join. The >> plan is to disable the question, however I suspect I've messed up the >> build system in that area, so this takes some more looking in to. > > Personally, I'm fine with the upstream "opt-in" nag warning. It only > shows up when you open the Syncthing web interface; it's not an > intrusive desktop "notification". Once the user has said "yes" or "no", > it doesn't appear again until Syncthing makes a change to what > information they collect. > > But, if people think our package should never ask, I don't mind if we > disable the request, as long as we share our changes with the Syncthing > project and they don't notice anything broken. I'll make an attempt at this while the jury is out, primarily to retrieve the ability to edit files. (Either I broke it, or I need to learn how to use (snippet). > Many of my comments below are about tedious things. Let us know if you > are getting sick of working on these packages, and I will help :) This > includes improving the descriptions. Yes! Help please. The meta-data part is tricky and time consuming, I'd rather spend my Guix time on the build system than descriptions etc. Synopsis and descriptions are just stuff i found on their home-page. >> Date: Thu, 15 Dec 2016 21:42:03 +0100 >> Subject: [PATCH] gnu: Add Syncthing. >> >> * gnu/packages/syncthing.scm: New file. > > Cool! > >> +(define-public syncthing > >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/syncthing/syncthing/") >> + (commit (string-append "v" version)))) >> + (file-name (string-append name "-" version >> "-checkout")) > > We should use the Syncthing release tarball: > > https://github.com/syncthing/syncthing/releases/download/v0.14.14/syncthing-source-v0.14.14.tar.gz Totally agree! However, tarballs are currently unsupported... due to, hm, let's say "because very good reasons noone can be blamed for, particularly and especially not me". Ok, I messed up the build system :P Removed too much code apparently. I'm working on getting this back. >> + (arguments >> + `(#:import-path "github.com/syncthing/syncthing" > > What do you think about having the go-build-system try to automatically > generate the import-path based on the source URL, with the option for > the packager to set it manually, as shown here? > > For many of the packages in this patch (which will eventually be split > into one package per patch ;) ), that auto-generated import-path > could be correct. > > I think that an (arguments) field indicates that the package's build > scripts have deviated from the standard. If a Guix build system > requires > all of its packages to do something in (arguments), the build system > should be extended :) This is my goal, and I tried to accomplish this initially, because as you can see for git checkouts most of the time import-path is url minus scheme://. But I was unable to retrieve the url in the build phases. So I did it like this instead, to get something that worked; also something like #:import-path is required where import-path can't be derived from the url. I would need concrete help with this, that is getting the url and perhaps what (method) was used. >> + #:phases >> + (modify-phases %standard-phases >> + (replace 'delete-files >> + (lambda _ >> + (delete-file-recursively >> "src/github.com/syncthing/syncthing/vendor"))) >> + >> + (replace 'build >> + (lambda* (#:key inputs #:allow-other-keys) >> + (with-directory-excursion >> "src/github.com/syncthing/syncthing" >> + (zero? (system* "go" "run" "build.go" "install" >> "syncthing" "-no-upgrade"))))) >> + >> + (replace 'install >> + (lambda _ >> + (copy-recursively >> "src/github.com/syncthing/syncthing/bin/" >> + (string-append (assoc-ref %outputs >> "out") "/bin")) >> + (copy-recursively "pkg" >> + (string-append (assoc-ref %outputs >> "out") "/pkg")) >> + (copy-recursively "src" >> + (string-append (assoc-ref %outputs >> "out") "/src"))))))) >> + > > Does this package need to use custom build and install phases? Yes. They use their own build program (build.go) and arguments. And the resulting binary is put in a non-standard location, "src/github.com/syncthing/syncthing/bin". If they had put it in "bin/" overriding this phase wouldn't be necessary. >> +(define-public golang-github-com-audriusbutkevicius-go-nat-pmp >> + (let ((commit "452c97607362b2ab5a7839b8d1704f0396b640ca")) > > Don't forget the revision counter :) Oh, I didn't see the need for it, with version being: "20160522.452c976". Should version really be: "20160522-1.452c976"? I don't see revision here contributing any value. >> +(define-public golang-github-com-bkaradzic-go-lz4 >> + (package >> + (name "golang-github-com-bkaradzic-go-lz4") >> + (version "1.0.0") >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/bkaradzic/go-lz4") >> + (commit >> "74ddf82598bc4745b965729e9c6a463bedd33049"))) > > For packages that we build from a Git tag (rather than an untagged > commit), you should do (commit (string-append "v" version)). Right. I've attached an updated patch with this. > Although, if there is a release tarball, it's preferable to use it. Sure, we can use tarballs when this is supported :P >> +(define-public golang-github-com-calmh-xdr >> + (let ((commit "f9b9f8f7aa27725f5cabb699bd9099ca7ce09143") >> + (revision "1")) >> + (package >> + (name "golang-github-com-calmh-xdr") >> + (version (string-append "2.0.0-" revision "." (string-take >> commit 7))) >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/calmh/xdr") >> + (commit commit))) > > You could also fetch this one with the Git tag, or use the tarball. > >> +(define-public golang-github-com-cznic-bufs >> + (let ((commit "3dcccbd7064a1689f9c093a988ea11ac00e21f51")) >> + (package >> + (name "golang-github-com-cznic-bufs") >> + (version (string-append "20160818." (string-take commit 7))) > > Is this string '20160818' the date of the commit? If the package has no > releases, we use '0.0.0' in place of the version. Yes. I figure this is best because some later project we'll package may be pinned to an earlier commit of a library, and then what? Incrementing the revision number would suggest this was of newer date, when it's not, and make the older commit appear as the newest version of the two to Guix. Projects not pinned to a library version will then use whatever commit was added last rather than the newest. >> + (arguments >> + `(#:import-path "github.com/cznic/internal/slice" >> + #:unpack-path "github.com/cznic/internal")) > > Is it too much to wonder if both the import-path and the unpack-path > could be auto-generated in cases like this? :) We should consider this together with the url: (url (string-append "https://github.com/cznic/internal")) "slice" is here a directory/package in the repository. For github.com we could probably assume that the repo is github.com/account/repo, but other domains may have different layouts. Also keeping in mind that the requested package could be a subpackage, "github.com/cznic/internal/slice/tricks". For github.com this would suggest we use url of the subpackage in the recipe, otherwise we don't have the import-path, but this url is not retreivable (404 Not Found). And then we would need for (git-fetch) to cut the url after the repo part. At this point I think we're at a place we don't want to be. I think the best we can do is only to skip #:unpack-path in these scenarios, and unpack according to domain and path of the url, as long as domain and path matches the start of the import-path. But as mentioned earlier, I don't know how to access the url in the build phases. I like that you're looking for simplifying the recipes, this is my goal too. And I'm sure we can automate more. Thanks for this thorough review! :) On 2016-12-16 02:52, Leo Famulari wrote: > On Thu, Dec 15, 2016 at 10:06:59PM +0100, Petter wrote: >> Hi again, >> >> I've prefixed most of the packages with "golang-" now. However, there >> are >> some >> packages already starting with "golang-", f.ex. >> "golang-org-x-text-unicode-norm", >> and I left those alone. It's inconsistent, but I felt this was >> preferable to >> names like "golang-golang-org-x-text-unicode-norm". What do you think? > > That's the right way. It fits the package naming guidelines: > > https://www.gnu.org/software/guix/manual/html_node/Package-Naming.html > >> Finally, there's a telemetry configuration in Syncthing. It is opt-in; >> but it will ask the user after a few hours if they want to join. The >> plan is to disable the question, however I suspect I've messed up the >> build system in that area, so this takes some more looking in to. > > Personally, I'm fine with the upstream "opt-in" nag warning. It only > shows up when you open the Syncthing web interface; it's not an > intrusive desktop "notification". Once the user has said "yes" or "no", > it doesn't appear again until Syncthing makes a change to what > information they collect. > > But, if people think our package should never ask, I don't mind if we > disable the request, as long as we share our changes with the Syncthing > project and they don't notice anything broken. > > Many of my comments below are about tedious things. Let us know if you > are getting sick of working on these packages, and I will help :) This > includes improving the descriptions. > >> Date: Thu, 15 Dec 2016 21:42:03 +0100 >> Subject: [PATCH] gnu: Add Syncthing. >> >> * gnu/packages/syncthing.scm: New file. > > Cool! > >> +(define-public syncthing > >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/syncthing/syncthing/") >> + (commit (string-append "v" version)))) >> + (file-name (string-append name "-" version >> "-checkout")) > > We should use the Syncthing release tarball: > > https://github.com/syncthing/syncthing/releases/download/v0.14.14/syncthing-source-v0.14.14.tar.gz > >> + (arguments >> + `(#:import-path "github.com/syncthing/syncthing" > > What do you think about having the go-build-system try to automatically > generate the import-path based on the source URL, with the option for > the packager to set it manually, as shown here? > > For many of the packages in this patch (which will eventually be split > into one package per patch ;) ), that auto-generated import-path > could be correct. > > I think that an (arguments) field indicates that the package's build > scripts have deviated from the standard. If a Guix build system > requires > all of its packages to do something in (arguments), the build system > should be extended :) > >> + #:phases >> + (modify-phases %standard-phases >> + (replace 'delete-files >> + (lambda _ >> + (delete-file-recursively >> "src/github.com/syncthing/syncthing/vendor"))) >> + >> + (replace 'build >> + (lambda* (#:key inputs #:allow-other-keys) >> + (with-directory-excursion >> "src/github.com/syncthing/syncthing" >> + (zero? (system* "go" "run" "build.go" "install" >> "syncthing" "-no-upgrade"))))) >> + >> + (replace 'install >> + (lambda _ >> + (copy-recursively >> "src/github.com/syncthing/syncthing/bin/" >> + (string-append (assoc-ref %outputs >> "out") "/bin")) >> + (copy-recursively "pkg" >> + (string-append (assoc-ref %outputs >> "out") "/pkg")) >> + (copy-recursively "src" >> + (string-append (assoc-ref %outputs >> "out") "/src"))))))) >> + > > Does this package need to use custom build and install phases? > >> +(define-public golang-github-com-audriusbutkevicius-go-nat-pmp >> + (let ((commit "452c97607362b2ab5a7839b8d1704f0396b640ca")) > > Don't forget the revision counter :) > >> +(define-public golang-github-com-bkaradzic-go-lz4 >> + (package >> + (name "golang-github-com-bkaradzic-go-lz4") >> + (version "1.0.0") >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/bkaradzic/go-lz4") >> + (commit >> "74ddf82598bc4745b965729e9c6a463bedd33049"))) > > For packages that we build from a Git tag (rather than an untagged > commit), you should do (commit (string-append "v" version)). > > Although, if there is a release tarball, it's preferable to use it. > >> +(define-public golang-github-com-calmh-xdr >> + (let ((commit "f9b9f8f7aa27725f5cabb699bd9099ca7ce09143") >> + (revision "1")) >> + (package >> + (name "golang-github-com-calmh-xdr") >> + (version (string-append "2.0.0-" revision "." (string-take >> commit 7))) >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/calmh/xdr") >> + (commit commit))) > > You could also fetch this one with the Git tag, or use the tarball. > >> +(define-public golang-github-com-cznic-bufs >> + (let ((commit "3dcccbd7064a1689f9c093a988ea11ac00e21f51")) >> + (package >> + (name "golang-github-com-cznic-bufs") >> + (version (string-append "20160818." (string-take commit 7))) > > Is this string '20160818' the date of the commit? If the package has no > releases, we use '0.0.0' in place of the version. > >> + (arguments >> + `(#:import-path "github.com/cznic/internal/slice" >> + #:unpack-path "github.com/cznic/internal")) > > Is it too much to wonder if both the import-path and the unpack-path > could be auto-generated in cases like this? :)