| Message ID | 20231210191046.4785-1-alex.kiernan@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] cargo-update-recipe-crates: Use CARGO_LOCK_PATH | expand |
I have to say, I struggle to see this as an improvement, and I want to object to it. Why set the path manually, if the code to find where Cargo.lock is just works, and handles the situations where upstreams move it around in source trees? Also, is the assumption that there's only one Cargo.lock valid? I'm not so sure. Note that the code was specifically written to pull in items from multiple files. Have you tested this change on all recipes in core and meta-oe that use the class? Alex On Sun, 10 Dec 2023 at 20:10, Alex Kiernan <alex.kiernan@gmail.com> wrote: > > Rather than searching for Cargo.lock, just use CARGO_LOCK_PATH, since we > have to know where the lock file is, else cargo_common_do_patch_paths > will fail. There should only ever be one lock file associated with a > project, so trying to aggregate across all of them makes no sense. > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > --- > > .../cargo-update-recipe-crates.bbclass | 30 +++++-------------- > 1 file changed, 8 insertions(+), 22 deletions(-) > > diff --git a/meta/classes-recipe/cargo-update-recipe-crates.bbclass b/meta/classes-recipe/cargo-update-recipe-crates.bbclass > index 8980137d02cf..59415ee8cb45 100644 > --- a/meta/classes-recipe/cargo-update-recipe-crates.bbclass > +++ b/meta/classes-recipe/cargo-update-recipe-crates.bbclass > @@ -4,6 +4,8 @@ > # SPDX-License-Identifier: MIT > # > > +inherit cargo_common > + > ## > ## Purpose: > ## This class is used to update the list of crates in SRC_URI > @@ -18,9 +20,6 @@ do_update_crates[depends] = "python3-native:do_populate_sysroot" > do_update_crates[nostamp] = "1" > do_update_crates[doc] = "Update the recipe by reading Cargo.lock and write in ${THISDIR}/${BPN}-crates.inc" > > -# The directory where to search for Cargo.lock files > -CARGO_LOCK_SRC_DIR ??= "${S}" > - > do_update_crates() { > TARGET_FILE="${THISDIR}/${BPN}-crates.inc" > > @@ -28,8 +27,7 @@ do_update_crates() { > > def get_crates(f): > import tomllib > - c_list = '# from %s' % os.path.relpath(f, '${CARGO_LOCK_SRC_DIR}') > - c_list += '\nSRC_URI += " \\\' > + c_list = 'SRC_URI += " \\\' > crates = tomllib.load(open(f, 'rb')) > > # Build a list with crates info that have crates.io in the source > @@ -55,23 +53,11 @@ def get_crates(f): > import os > crates = "# Autogenerated with 'bitbake -c update_crates ${PN}'\n\n" > found = False > -for root, dirs, files in os.walk('${CARGO_LOCK_SRC_DIR}'): > - # ignore git and patches directories > - if root.startswith(os.path.join('${CARGO_LOCK_SRC_DIR}', '.pc')): > - continue > - if root.startswith(os.path.join('${CARGO_LOCK_SRC_DIR}', '.git')): > - continue > - for file in files: > - if file == 'Cargo.lock': > - try: > - cargo_lock_path = os.path.join(root, file) > - crates += get_crates(os.path.join(root, file)) > - except Exception as e: > - raise ValueError("Cannot parse '%s'" % cargo_lock_path) from e > - else: > - found = True > -if not found: > - raise ValueError("Unable to find any Cargo.lock in ${CARGO_LOCK_SRC_DIR}") > +try: > + cargo_lock_path = '${CARGO_LOCK_PATH}' > + crates += get_crates(cargo_lock_path) > +except Exception as e: > + raise ValueError("Cannot parse '%s'" % cargo_lock_path) from e > open("${TARGET_FILE}", 'w').write(crates) > EOF > > -- > 2.39.0 > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#192116): https://lists.openembedded.org/g/openembedded-core/message/192116 > Mute This Topic: https://lists.openembedded.org/mt/103094777/1686489 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [alex.kanavin@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- >
On Sun, 10 Dec 2023 at 21:35, Alexander Kanavin <alex.kanavin@gmail.com> wrote: > I have to say, I struggle to see this as an improvement, and I want > to object to it. > > Why set the path manually, if the code to find where Cargo.lock is > just works, and handles the situations where upstreams move it around > in source trees? > > Also, is the assumption that there's only one Cargo.lock valid? I'm > not so sure. Note that the code was specifically written to pull in > items from multiple files. > I think the assumption of a single Cargo.lock is valid. For workspace project (so project which generate multiple artifacts) , it is explicitly said: "A workspace is a set of packages that share the same Cargo.lock and output directory" (taken from https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html) And the FAQ ( https://doc.rust-lang.org/cargo/guide/cargo-toml-vs-cargo-lock.html) doesn't mention Cargo.lock in plural. > Have you tested this change on all recipes in core and meta-oe that > use the class? > > Alex > > On Sun, 10 Dec 2023 at 20:10, Alex Kiernan <alex.kiernan@gmail.com> wrote: > > > > Rather than searching for Cargo.lock, just use CARGO_LOCK_PATH, since we > > have to know where the lock file is, else cargo_common_do_patch_paths > > will fail. There should only ever be one lock file associated with a > > project, so trying to aggregate across all of them makes no sense. > > > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > > --- > > > > .../cargo-update-recipe-crates.bbclass | 30 +++++-------------- > > 1 file changed, 8 insertions(+), 22 deletions(-) > > > > diff --git a/meta/classes-recipe/cargo-update-recipe-crates.bbclass > b/meta/classes-recipe/cargo-update-recipe-crates.bbclass > > index 8980137d02cf..59415ee8cb45 100644 > > --- a/meta/classes-recipe/cargo-update-recipe-crates.bbclass > > +++ b/meta/classes-recipe/cargo-update-recipe-crates.bbclass > > @@ -4,6 +4,8 @@ > > # SPDX-License-Identifier: MIT > > # > > > > +inherit cargo_common > > + > > ## > > ## Purpose: > > ## This class is used to update the list of crates in SRC_URI > > @@ -18,9 +20,6 @@ do_update_crates[depends] = > "python3-native:do_populate_sysroot" > > do_update_crates[nostamp] = "1" > > do_update_crates[doc] = "Update the recipe by reading Cargo.lock and > write in ${THISDIR}/${BPN}-crates.inc" > > > > -# The directory where to search for Cargo.lock files > > -CARGO_LOCK_SRC_DIR ??= "${S}" > > - > > do_update_crates() { > > TARGET_FILE="${THISDIR}/${BPN}-crates.inc" > > > > @@ -28,8 +27,7 @@ do_update_crates() { > > > > def get_crates(f): > > import tomllib > > - c_list = '# from %s' % os.path.relpath(f, '${CARGO_LOCK_SRC_DIR}') > > - c_list += '\nSRC_URI += " \\\' > > + c_list = 'SRC_URI += " \\\' > > crates = tomllib.load(open(f, 'rb')) > > > > # Build a list with crates info that have crates.io in the source > > @@ -55,23 +53,11 @@ def get_crates(f): > > import os > > crates = "# Autogenerated with 'bitbake -c update_crates ${PN}'\n\n" > > found = False > > -for root, dirs, files in os.walk('${CARGO_LOCK_SRC_DIR}'): > > - # ignore git and patches directories > > - if root.startswith(os.path.join('${CARGO_LOCK_SRC_DIR}', '.pc')): > > - continue > > - if root.startswith(os.path.join('${CARGO_LOCK_SRC_DIR}', '.git')): > > - continue > > - for file in files: > > - if file == 'Cargo.lock': > > - try: > > - cargo_lock_path = os.path.join(root, file) > > - crates += get_crates(os.path.join(root, file)) > > - except Exception as e: > > - raise ValueError("Cannot parse '%s'" % cargo_lock_path) > from e > > - else: > > - found = True > > -if not found: > > - raise ValueError("Unable to find any Cargo.lock in > ${CARGO_LOCK_SRC_DIR}") > > +try: > > + cargo_lock_path = '${CARGO_LOCK_PATH}' > > + crates += get_crates(cargo_lock_path) > > +except Exception as e: > > + raise ValueError("Cannot parse '%s'" % cargo_lock_path) from e > > open("${TARGET_FILE}", 'w').write(crates) > > EOF > > > > -- > > 2.39.0 > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#192126): > https://lists.openembedded.org/g/openembedded-core/message/192126 > Mute This Topic: https://lists.openembedded.org/mt/103094777/6213388 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [ > frederic.martinsons@gmail.com] > -=-=-=-=-=-=-=-=-=-=-=- > >
On Sun, Dec 10, 2023 at 8:35 PM Alexander Kanavin <alex.kanavin@gmail.com> wrote: > > I have to say, I struggle to see this as an improvement, and I want > to object to it. > I have to admit I kinda thought this might be controversial. > Why set the path manually, if the code to find where Cargo.lock is > just works, and handles the situations where upstreams move it around > in source trees? > We have to know where Cargo.lock is else cargo_common_do_patch_paths fails. Searching for it one class and requiring to know where it is another seems odd to me - my first pass on this change was to just set CARGO_LOCK_SRC_DIR based on the directory name of CARGO_LOCK_PATH; I'll resurrect that patch. > Also, is the assumption that there's only one Cargo.lock valid? I'm > not so sure. Note that the code was specifically written to pull in > items from multiple files. > In a single project there are (potentially) multiple Cargo.toml, but just one Cargo.lock. If there's multiple Cargo.lock files in a repo, that implies multiple projects which I don't think we've any way of building from a single recipe.
On Mon, 11 Dec 2023 at 10:09, Alex Kiernan <alex.kiernan@gmail.com> wrote: > In a single project there are (potentially) multiple Cargo.toml, but > just one Cargo.lock. If there's multiple Cargo.lock files in a repo, > that implies multiple projects which I don't think we've any way of > building from a single recipe. I'm thinking of projects like librsvg, python3-bcrypt or python3-cryptography where the build is orchestrated by meson, autoconf or setuptools, and where we don't run cargo directly and leave it to those tools. They may have single Cargo.lock now, but will that hold? Also, that Cargo.lock is not necessarily in the root of ${S}, so you'd have to set the location manually, instead of not having to worry about it. Is CARGO_LOCK_PATH or its parent variables already set correctly for all of them? I see this is not done from python3-cryptography; shouldn't the build error out, if cargo_common_do_patch_paths requires the location? Something doesn't compute fully here. Alex
On Mon, Dec 11, 2023 at 9:28 AM Alexander Kanavin <alex.kanavin@gmail.com> wrote: > > On Mon, 11 Dec 2023 at 10:09, Alex Kiernan <alex.kiernan@gmail.com> wrote: > > In a single project there are (potentially) multiple Cargo.toml, but > > just one Cargo.lock. If there's multiple Cargo.lock files in a repo, > > that implies multiple projects which I don't think we've any way of > > building from a single recipe. > > I'm thinking of projects like librsvg, python3-bcrypt or > python3-cryptography where the build is orchestrated by meson, > autoconf or setuptools, and where we don't run cargo directly and > leave it to those tools. They may have single Cargo.lock now, but will > that hold? Also, that Cargo.lock is not necessarily in the root of > ${S}, so you'd have to set the location manually, instead of not > having to worry about it. Is CARGO_LOCK_PATH or its parent variables > already set correctly for all of them? I see this is not done from > python3-cryptography; shouldn't the build error out, if > cargo_common_do_patch_paths requires the location? Something doesn't > compute fully here. > That's exactly the one I was going to look at next, as I agree something doesn't add up there.
On Mon, Dec 11, 2023 at 9:43 AM Alex Kiernan <alex.kiernan@gmail.com> wrote: > > On Mon, Dec 11, 2023 at 9:28 AM Alexander Kanavin > <alex.kanavin@gmail.com> wrote: > > > > On Mon, 11 Dec 2023 at 10:09, Alex Kiernan <alex.kiernan@gmail.com> wrote: > > > In a single project there are (potentially) multiple Cargo.toml, but > > > just one Cargo.lock. If there's multiple Cargo.lock files in a repo, > > > that implies multiple projects which I don't think we've any way of > > > building from a single recipe. > > > > I'm thinking of projects like librsvg, python3-bcrypt or > > python3-cryptography where the build is orchestrated by meson, > > autoconf or setuptools, and where we don't run cargo directly and > > leave it to those tools. They may have single Cargo.lock now, but will > > that hold? Also, that Cargo.lock is not necessarily in the root of > > ${S}, so you'd have to set the location manually, instead of not > > having to worry about it. Is CARGO_LOCK_PATH or its parent variables > > already set correctly for all of them? I see this is not done from > > python3-cryptography; shouldn't the build error out, if > > cargo_common_do_patch_paths requires the location? Something doesn't > > compute fully here. > > > > That's exactly the one I was going to look at next, as I agree > something doesn't add up there. > cargo_common_do_patch_paths isn't in the call graph for python3-cryptography, it gets patched in like this: do_configure[postfuncs] += "cargo_common_do_patch_paths" I'm unclear why that doesn't execute, I'm wondering if its related to the autogenerated do_configure and this is a usage that just doesn't work? # line: 1, file: autogenerated do_configure() { setup_target_config python_setuptools3_rust_do_configure } However even if it was in the call graph, we wouldn't have errored in it as there's no patches to apply to the cargo config as the crates are all satisfied from the vendored sources we deliver. It does feel like we're missing a piece in our python/rust integration since if you did have a crate delivered from git then we wouldn't patch up the cargo config correctly and I guess end up erroring out. -- Alex Kiernan
On Mon, Dec 11, 2023 at 10:52 AM Alex Kiernan via lists.openembedded.org <alex.kiernan=gmail.com@lists.openembedded.org> wrote: > > On Mon, Dec 11, 2023 at 9:43 AM Alex Kiernan <alex.kiernan@gmail.com> wrote: > > > > On Mon, Dec 11, 2023 at 9:28 AM Alexander Kanavin > > <alex.kanavin@gmail.com> wrote: > > > > > > On Mon, 11 Dec 2023 at 10:09, Alex Kiernan <alex.kiernan@gmail.com> wrote: > > > > In a single project there are (potentially) multiple Cargo.toml, but > > > > just one Cargo.lock. If there's multiple Cargo.lock files in a repo, > > > > that implies multiple projects which I don't think we've any way of > > > > building from a single recipe. > > > > > > I'm thinking of projects like librsvg, python3-bcrypt or > > > python3-cryptography where the build is orchestrated by meson, > > > autoconf or setuptools, and where we don't run cargo directly and > > > leave it to those tools. They may have single Cargo.lock now, but will > > > that hold? Also, that Cargo.lock is not necessarily in the root of > > > ${S}, so you'd have to set the location manually, instead of not > > > having to worry about it. Is CARGO_LOCK_PATH or its parent variables > > > already set correctly for all of them? I see this is not done from > > > python3-cryptography; shouldn't the build error out, if > > > cargo_common_do_patch_paths requires the location? Something doesn't > > > compute fully here. > > > > > > > That's exactly the one I was going to look at next, as I agree > > something doesn't add up there. > > > > cargo_common_do_patch_paths isn't in the call graph for > python3-cryptography, it gets patched in like this: > > do_configure[postfuncs] += "cargo_common_do_patch_paths" > > I'm unclear why that doesn't execute, I'm wondering if its related to > the autogenerated do_configure and this is a usage that just doesn't > work? > > # line: 1, file: autogenerated > do_configure() { > setup_target_config > python_setuptools3_rust_do_configure > } > > However even if it was in the call graph, we wouldn't have errored in > it as there's no patches to apply to the cargo config as the crates > are all satisfied from the vendored sources we deliver. > > It does feel like we're missing a piece in our python/rust integration > since if you did have a crate delivered from git then we wouldn't > patch up the cargo config correctly and I guess end up erroring out. > Okay, I'm wrong. The pieces are all there, just we have no git crates to patch in for python3-cryptography so the existence check for Cargo.lock never runs. Maybe that should be hoisted earlier in the function so you see the failure if Cargo.lock isn't where it would be expected to be.
diff --git a/meta/classes-recipe/cargo-update-recipe-crates.bbclass b/meta/classes-recipe/cargo-update-recipe-crates.bbclass index 8980137d02cf..59415ee8cb45 100644 --- a/meta/classes-recipe/cargo-update-recipe-crates.bbclass +++ b/meta/classes-recipe/cargo-update-recipe-crates.bbclass @@ -4,6 +4,8 @@ # SPDX-License-Identifier: MIT # +inherit cargo_common + ## ## Purpose: ## This class is used to update the list of crates in SRC_URI @@ -18,9 +20,6 @@ do_update_crates[depends] = "python3-native:do_populate_sysroot" do_update_crates[nostamp] = "1" do_update_crates[doc] = "Update the recipe by reading Cargo.lock and write in ${THISDIR}/${BPN}-crates.inc" -# The directory where to search for Cargo.lock files -CARGO_LOCK_SRC_DIR ??= "${S}" - do_update_crates() { TARGET_FILE="${THISDIR}/${BPN}-crates.inc" @@ -28,8 +27,7 @@ do_update_crates() { def get_crates(f): import tomllib - c_list = '# from %s' % os.path.relpath(f, '${CARGO_LOCK_SRC_DIR}') - c_list += '\nSRC_URI += " \\\' + c_list = 'SRC_URI += " \\\' crates = tomllib.load(open(f, 'rb')) # Build a list with crates info that have crates.io in the source @@ -55,23 +53,11 @@ def get_crates(f): import os crates = "# Autogenerated with 'bitbake -c update_crates ${PN}'\n\n" found = False -for root, dirs, files in os.walk('${CARGO_LOCK_SRC_DIR}'): - # ignore git and patches directories - if root.startswith(os.path.join('${CARGO_LOCK_SRC_DIR}', '.pc')): - continue - if root.startswith(os.path.join('${CARGO_LOCK_SRC_DIR}', '.git')): - continue - for file in files: - if file == 'Cargo.lock': - try: - cargo_lock_path = os.path.join(root, file) - crates += get_crates(os.path.join(root, file)) - except Exception as e: - raise ValueError("Cannot parse '%s'" % cargo_lock_path) from e - else: - found = True -if not found: - raise ValueError("Unable to find any Cargo.lock in ${CARGO_LOCK_SRC_DIR}") +try: + cargo_lock_path = '${CARGO_LOCK_PATH}' + crates += get_crates(cargo_lock_path) +except Exception as e: + raise ValueError("Cannot parse '%s'" % cargo_lock_path) from e open("${TARGET_FILE}", 'w').write(crates) EOF
Rather than searching for Cargo.lock, just use CARGO_LOCK_PATH, since we have to know where the lock file is, else cargo_common_do_patch_paths will fail. There should only ever be one lock file associated with a project, so trying to aggregate across all of them makes no sense. Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> --- .../cargo-update-recipe-crates.bbclass | 30 +++++-------------- 1 file changed, 8 insertions(+), 22 deletions(-)