| Message ID | 8981451ac5d914dd5f53fa928741b846@riseup.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#38408] WIP patches for the rust importer | expand |
On Fri, Nov 29, 2019 at 07:59:35AM -0800, Martin Becze wrote: > On 2019-11-28 12:22, Efraim Flashner wrote: > > On Wed, Nov 27, 2019 at 04:36:20PM -0800, mjbecze@riseup.net wrote: > >> > >> > I'd love to see what you have so far if you want to share > >> > >> Okie Dokie, I posted it and cc'd ya. > >> > >> Also I took a look at your patches. > >> 0001-import-crate-Don-t-include-optional-dependencies.patch should work > >> just fine with my patch. But > >> 0003-import-crate-Honor-versioned-dependencies-when-impor.patch will not > >> work. > >> > >> I took a different route here with the naming. If you are interested take > >> a look take a look at my second patch. (recusive-import-semver) only will > >> add the version number to the name if the crate being imported is not the > >> latest version. I thought this was more inline with the canonical names, > >> but if always adding version number the export symbol is desirable it will > >> simplify things. > >> > > > > I'll take a look at it in a minute. I figured with the versioned > > requirements we would always want to be specific in version numbers for > > crate dependents so I figured it made sense. Also, if we did want to > > provide an unversioned '-latest' version we could declare an extra > > variable '(define-public rust-libc rust-libc-0.2)' and now rust-libc > > points to rust-libc-0.2. > > > >> Also I added a function (find-packages-by-name*/direct) to packages.scm > >> which will return the export symbol of a package that already exists. I > >> use this in case there are some non-canocal export name already added. > >> > > I added the no-optional-dep logic to the recusive-semver patch > (https://issues.guix.gnu.org/issue/38408), but it seems to break > packages. I'm testing on the recursive importer on "hello-cli". Attach > is the patch to add the logic and the before and after output for "guix > import crate -r hello-cli". Removing all the optional deps breaks clap > here for some reason which I haven't figured out. Looking at the two attached files I want to bring attention to the size of them: after.scm [text/plain, base64, utf-8, 5.7K] before.scm [text/plain, base64, utf-8, 226K] One way to fix this (in addition to your "default to the don't build" argument) is to keep rust-clap in it's broken state and define a different rust-clap for when we actually want it and to have that use enough inputs to actually build. I do now actually realize that only really works if we keep all the crates hidden, which I don't think we want. So I guess that has us sending bug reports upstream that some of the optional dependencies aren't actually optional. In any case, I think it'd be better to skip the optional dependencies and then add them back in as needed to actually build the crates we want.
On 2019-12-01 08:59, Efraim Flashner wrote: > On Fri, Nov 29, 2019 at 07:59:35AM -0800, Martin Becze wrote: >> On 2019-11-28 12:22, Efraim Flashner wrote: >> > On Wed, Nov 27, 2019 at 04:36:20PM -0800, mjbecze@riseup.net wrote: >> >> >> >> > I'd love to see what you have so far if you want to share >> >> >> >> Okie Dokie, I posted it and cc'd ya. >> >> >> >> Also I took a look at your patches. >> >> 0001-import-crate-Don-t-include-optional-dependencies.patch should work >> >> just fine with my patch. But >> >> 0003-import-crate-Honor-versioned-dependencies-when-impor.patch will not >> >> work. >> >> >> >> I took a different route here with the naming. If you are interested take >> >> a look take a look at my second patch. (recusive-import-semver) only will >> >> add the version number to the name if the crate being imported is not the >> >> latest version. I thought this was more inline with the canonical names, >> >> but if always adding version number the export symbol is desirable it will >> >> simplify things. >> >> >> > >> > I'll take a look at it in a minute. I figured with the versioned >> > requirements we would always want to be specific in version numbers for >> > crate dependents so I figured it made sense. Also, if we did want to >> > provide an unversioned '-latest' version we could declare an extra >> > variable '(define-public rust-libc rust-libc-0.2)' and now rust-libc >> > points to rust-libc-0.2. >> > >> >> Also I added a function (find-packages-by-name*/direct) to packages.scm >> >> which will return the export symbol of a package that already exists. I >> >> use this in case there are some non-canocal export name already added. >> >> >> >> I added the no-optional-dep logic to the recusive-semver patch >> (https://issues.guix.gnu.org/issue/38408), but it seems to break >> packages. I'm testing on the recursive importer on "hello-cli". Attach >> is the patch to add the logic and the before and after output for "guix >> import crate -r hello-cli". Removing all the optional deps breaks clap >> here for some reason which I haven't figured out. > > Looking at the two attached files I want to bring attention to the size > of them: > after.scm [text/plain, base64, utf-8, 5.7K] > before.scm [text/plain, base64, utf-8, 226K] > > One way to fix this (in addition to your "default to the don't build" > argument) is to keep rust-clap in it's broken state and define a > different rust-clap for when we actually want it and to have that use > enough inputs to actually build. I do now actually realize that only > really works if we keep all the crates hidden, which I don't think we > want. So I guess that has us sending bug reports upstream that some of > the optional dependencies aren't actually optional. > > In any case, I think it'd be better to skip the optional dependencies > and then add them back in as needed to actually build the crates we > want. oh to be more clear. Even with "#:skip-build? #t" on all the rust libs, things fail if I omit one optional dependency from somewhere. I'm just testing on hello-cli but hello-cli inculdes clap which has alot of optional deps. In ./guix/build-systems/cargo.scm on line 186 it says "Cargo requires all transitive crate dependencies' sources to be available in its index, even if they are optional (this is so it can generate deterministic Cargo.lock files regardless of the target platform or enabled features). Thus we need all transitive crate dependencies for any cargo dev-dependencies, but this is only needed when building/testing a crate directly (i.e. we will never need transitive dev-dependencies for any dependency crates)." I haven't really dug into the cargo build-system yet but my guess is that the problem lies there. Allllsooo i totally could have missed something simple.. idk
Hi Martin! > On Dec 1, 2019, at 7:17 PM, Martin Becze <mjbecze@riseup.net> wrote: > > oh to be more clear. Even with "#:skip-build? #t" on all the rust libs, > things fail if I omit one optional dependency from somewhere. I'm just > testing on hello-cli but hello-cli inculdes clap which has alot of > optional deps. In ./guix/build-systems/cargo.scm on line 186 it says > > "Cargo requires all transitive crate dependencies' sources to be > available > in its index, even if they are optional (this is so it can generate > deterministic Cargo.lock files regardless of the target platform or > enabled > features). Thus we need all transitive crate dependencies for any cargo > dev-dependencies, but this is only needed when building/testing a crate > directly > (i.e. we will never need transitive dev-dependencies for any dependency > crates)." > > I haven't really dug into the cargo build-system yet but my guess is > that the problem lies there. Allllsooo i totally could have missed > something simple.. idk Yes, this is the primary limitation of packaging rust crates into guix without manually needing to edit the Cargo.toml definition. My opinion is that the easiest (to maintain) method for incorporating rust packages into guix is to bite the bullet and perform a (source) import of all transitive dependencies that a package might need. Manually keeping up with tweaking the Cargo.toml file for every single potential package, across every single published version will make you go mad :) Then the challenge would be how to automate the incremental importing from crates.io <http://crates.io/> (for example, some crates require a specific x.y.z version of a dependency, some are willing to work with x.y.*, some have ranges, etc.) and making sure that all packaged crates point to the appropriate dependencies as they get updated in guix. —Ivan
On 2019-12-02 04:01, Ivan Petkov wrote: > Hi Martin! > >> On Dec 1, 2019, at 7:17 PM, Martin Becze <mjbecze@riseup.net> wrote: >> >> oh to be more clear. Even with "#:skip-build? #t" on all the rust >> libs, >> things fail if I omit one optional dependency from somewhere. I'm >> just >> testing on hello-cli but hello-cli inculdes clap which has alot of >> optional deps. In ./guix/build-systems/cargo.scm on line 186 it says >> >> >> "Cargo requires all transitive crate dependencies' sources to be >> available >> in its index, even if they are optional (this is so it can generate >> deterministic Cargo.lock files regardless of the target platform or >> enabled >> features). Thus we need all transitive crate dependencies for any >> cargo >> dev-dependencies, but this is only needed when building/testing a >> crate >> directly >> (i.e. we will never need transitive dev-dependencies for any >> dependency >> crates)." >> >> I haven't really dug into the cargo build-system yet but my guess is >> that the problem lies there. Allllsooo i totally could have missed >> something simple.. idk > > Yes, this is the primary limitation of packaging rust crates into guix > without manually needing to edit the Cargo.toml definition. > > My opinion is that the easiest (to maintain) method for incorporating > rust packages into guix is to bite the bullet and perform a (source) > import of all transitive dependencies that a package might need. > Manually keeping up with tweaking the Cargo.toml file for every single > potential package, across every single published version will make > you go mad :) > > Then the challenge would be how to automate the incremental importing > from crates.io [1] (for example, some crates require a specific x.y.z > version of > a dependency, some are willing to work with x.y.*, some have ranges, > etc.) > and making sure that all packaged crates point to the appropriate > dependencies > as they get updated in guix. > > —Ivan > > Links: > ------ > [1] http://crates.io Hi Ivan, > My opinion is that the easiest (to maintain) method for incorporating > rust packages into guix is to bite the bullet and perform a (source) > import of all transitive dependencies that a package might need. When you say source import of the transitive dependencies, do you mean that all the rust libs should just be source only or do you mean the top level package should have to declare all the transitive dependencies? If former I agree but if it is the latter then I would consider rust packaging to be broken. > Then the challenge would be how to automate the incremental importing > from crates.io [1] (for example, some crates require a specific x.y.z > version of > a dependency, some are willing to work with x.y.*, some have ranges, > etc.) Yes that is what this patch (https://issues.guix.gnu.org/issue/38408) does. It uses semantic versioning to select the correct package form crate.io or if available from the alread packaged rust packages. -Martin
Hi Martin, > On Dec 2, 2019, at 3:10 PM, Martin Becze <mjbecze@riseup.net> wrote: > > When you say source import of the transitive dependencies, do you mean > that all the rust libs should just be source only or do you mean the top > level package should have to declare all the transitive dependencies? All rust libs should be source only imports, but each package definition should only declare dependencies on the crates it consumes directly and guix should figure out the rest (in other words, I’d expect there to be a one-to-one mapping between a Cargo.toml and a package definition). For example, if crate foo depends on crate bar which depends on crate baz, I’d expect the definitions to look like: (define-public rust-foo (package (name “rust-foo") (source-input `((“bar” ,bar))))) (define-public rust-bar (package (name “rust-bar") (source-input `((“baz” ,baz))))) (define-public rust-baz (package (name “rust-baz"))) But while building foo (assuming it is some kind of application), guix would ensure that bar and baz are available in the build environment. IMO this direction would be the most maintainable in the long term. —Ivan
On 2019-12-04 02:40, Ivan Petkov wrote: > Hi Martin, > >> On Dec 2, 2019, at 3:10 PM, Martin Becze <mjbecze@riseup.net> wrote: >> >> When you say source import of the transitive dependencies, do you >> mean >> that all the rust libs should just be source only or do you mean the >> top >> level package should have to declare all the transitive >> dependencies? > > All rust libs should be source only imports, but each package > definition > should only declare dependencies on the crates it consumes directly > and > guix should figure out the rest (in other words, I’d expect there to > be a > one-to-one mapping between a Cargo.toml and a package definition). > > For example, if crate foo depends on crate bar which depends on crate > baz, I’d expect the definitions to look like: > > (define-public rust-foo > (package > (name “rust-foo") > (source-input `((“bar” ,bar))))) > > (define-public rust-bar > (package > (name “rust-bar") > (source-input `((“baz” ,baz))))) > > (define-public rust-baz > (package > (name “rust-baz"))) > > But while building foo (assuming it is some kind of application), guix > would ensure that bar and baz are available in the build environment. > > IMO this direction would be the most maintainable in the long term. > > —Ivan Yes agree and that is what (recusive-import-semver) for produces rust. -Martin
diff --git a/guix/import/crate.scm b/guix/import/crate.scm index da92c43b8c..355aaa140a 100644 --- a/guix/import/crate.scm +++ b/guix/import/crate.scm @@ -2,6 +2,7 @@ ;;; Copyright © 2016 David Craven <david@craven.ch> ;;; Copyright © 2019 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2019 Martin Becze <mjbecze@riseup.net> +;;; Copyright © 2019 Efraim Flashner <efraim@flashner.co.il> ;;; ;;; This file is part of GNU Guix. ;;; @@ -88,6 +89,7 @@ (id crate-dependency-id "crate_id") ;string (kind crate-dependency-kind "kind" ;'normal | 'dev | 'build string->symbol) + (optional crate-dependency-optional "optional") ; 'true | 'false (requirement crate-dependency-requirement "req")) ;string (define (lookup-crate name) @@ -108,12 +110,16 @@ record or #f if it was not found." (define (crate-version-dependencies version) "Return the list of <crate-dependency> records of VERSION, a <crate-version>." + (define (optional-dependency? dependency) + (eq? (crate-dependency-optional dependency) #t)) + (let* ((path (assoc-ref (crate-version-links version) "dependencies")) (url (string-append (%crate-base-url) path))) (match (assoc-ref (or (json-fetch url) '()) "dependencies") ((? vector? vector) (filter (lambda (dep) - (not (eq? (crate-dependency-kind dep) 'dev))) + (not (or (eq? (crate-dependency-kind dep) 'dev) + (optional-dependency? dep)))) (map json->crate-dependency (vector->list vector)))) (_ '()))))