Skip to content

superflore-gen-oe-recipes: Delete all generated files when regenerating all recipes#265

Merged
herb-kuta-lge merged 1 commit intoros-infrastructure:masterfrom
shr-project:jansa/ros-clean
Apr 22, 2020
Merged

superflore-gen-oe-recipes: Delete all generated files when regenerating all recipes#265
herb-kuta-lge merged 1 commit intoros-infrastructure:masterfrom
shr-project:jansa/ros-clean

Conversation

@shr-project
Copy link
Contributor

@shr-project shr-project commented Feb 5, 2020

  • the overlay.repo.remove_file(existing, True) call in bitbake/gen_packages.py
    works fine, but only when the new recipe is being generated in the same directory
    as the previous version:
    existing = glob.glob('{}_*.bb'.format(prefix))
    where prefix is:
    prefix = '{0}/meta-ros{1}-{2}/generated-recipes/{3}/{4}'
    which will result in multiple recipes for the same package when the
    package is moved to different prefix like pcl-conversions did for
    dashing and eloquent:
  meta-ros$ find . -name pcl-conversions\*bb
  ./meta-ros2-eloquent/generated-recipes/perception-pcl/pcl-conversions_2.1.0-1.bb
  ./meta-ros2-eloquent/generated-recipes/pcl-conversions/pcl-conversions_2.0.0-1.bb
  ./meta-ros1-melodic/generated-recipes/perception-pcl/pcl-conversions_1.7.0-2.bb
  ./meta-ros2-crystal/generated-recipes/pcl-conversions/pcl-conversions_2.0.0.bb
  ./meta-ros2-dashing/generated-recipes/perception-pcl/pcl-conversions_2.0.0-1.bb
  ./meta-ros2-dashing/generated-recipes/pcl-conversions/pcl-conversions_2.0.0-1.bb

and especially for dashing where the version is identical it's
undeterministic which recipe bitbake will use. And the recipe wasn't
just moved to different directory, it's quite different:

diff ./meta-ros2-dashing/generated-recipes/perception-pcl/pcl-conversions_2.0.0-1.bb ./meta-ros2-dashing/generated-recipes/pcl-conversions/pcl-conversions_2.0.0-1.bb
9c9
< AUTHOR = "Paul Bovbel <paul@bovbel.com>"
---
> AUTHOR = "Chris Lalancette <clalancette@openrobotics.org>"
14c14
< LIC_FILES_CHKSUM = "file://package.xml;beginline=16;endline=16;md5=d566ef916e9dedc494f5f793a6690ba5"
---
> LIC_FILES_CHKSUM = "file://package.xml;beginline=12;endline=12;md5=d566ef916e9dedc494f5f793a6690ba5"
16c16
< ROS_CN = "perception_pcl"
---
> ROS_CN = "pcl_conversions"
19a20
>     builtin-interfaces \
21d21
<     message-filters \
23,24d22
<     pcl-msgs \
<     rclcpp \
33a32
>     builtin-interfaces \
35d33
<     message-filters \
37,38d34
<     pcl-msgs \
<     rclcpp \
46,47d41
<     libeigen \
<     message-filters \
49,52d42
<     pcl-msgs \
<     rclcpp \
<     sensor-msgs \
<     std-msgs \
67c57
< # matches with: https://github.com/ros2-gbp/perception_pcl-release/archive/release/dashing/pcl_conversions/2.0.0-1.tar.gz
---
> # matches with: https://github.com/ros2-gbp/pcl_conversions-release/archive/release/dashing/pcl_conversions/2.0.0-1.tar.gz
69,70c59,60
< SRC_URI = "git://github.com/ros2-gbp/perception_pcl-release;${ROS_BRANCH};protocol=https"
< SRCREV = "0f550985da8bd73fd7451350c63e1ce96e78c955"
---
> SRC_URI = "git://github.com/ros2-gbp/pcl_conversions-release;${ROS_BRANCH};protocol=https"
> SRCREV = "8d0d0ef7d5d253f11fbf52388d64fad59e46377a"
  • grep in the current build logs shows that the "older"
    meta-ros2-dashing/generated-recipes/pcl-conversions/pcl-conversions_2.0.0-1.bb
    was currently being used in dashing instead of "newer"
    meta-ros2-dashing/generated-recipes/perception-pcl/pcl-conversions_2.0.0-1.bb

  • corresponding rosdistro change for dashing:
    ros/rosdistro@19b06c4
    and eloquent:
    ros/rosdistro@67c3ef5

Signed-off-by: Martin Jansa martin.jansa@lge.com

Copy link
Contributor

@herb-kuta-lge herb-kuta-lge left a comment

Choose a reason for hiding this comment

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

Suggested change to commit summary:

bitbake: Delete all generated files when regenerating all recipes

I still need to review the actual code changes.

@herb-kuta-lge herb-kuta-lge changed the title bitbake: delete all generated files in clean_ros_recipe_dirs and call… Delete all generated files when regenerating all recipes Mar 19, 2020
@herb-kuta-lge herb-kuta-lge changed the title Delete all generated files when regenerating all recipes superflore-gen-oe-recipes: Delete all generated files when regenerating all recipes Mar 19, 2020
# D meta-ros2-eloquent/generated-recipes/variants/ros-base_0.8.3-1.bb
# we want just the path with filename
existing = existing.split()[1]
if not existing:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can line 62 ever result it existing evaluting to False? If not, then shouldn't this line be else:?

@herb-kuta-lge herb-kuta-lge self-requested a review March 19, 2020 19:42
Copy link
Contributor

@herb-kuta-lge herb-kuta-lge left a comment

Choose a reason for hiding this comment

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

Commit message change suggestions:

clean_ros_recipe_dirs wasn't called anywhere, extend it and call this when regenerating all recipes

Replace with:
clean_ros_recipe_dirs() wasn't called anywhere, update it for what's now generated by superflore and call it when regenerating all of the recipes.

the package is moved to different prefix like pcl-conversions did for dashing and eloquent

Replace "package is moved to different prefix like pcl-conversions did for" with "component that provides a package is changed like what happened to pcl-conversions in".

./meta-ros2-crystal/generated-recipes/pcl-conversions/pcl-conversions_2.0.0.bb

Drop.

And the recipe wasn't just moved to different directory, it's quite different:

Drop this and what follows. I don't see how showing how different the recipes are helps further explain the need for the change.

@shr-project shr-project force-pushed the jansa/ros-clean branch 2 times, most recently from faa1967 to 513fd88 Compare March 23, 2020 13:47
@shr-project
Copy link
Contributor Author

Drop this and what follows. I don't see how showing how different the recipes are helps further explain the need for the change.

I disagree, It shows why it's important to implement this change.

@herb-kuta-lge
Copy link
Contributor

herb-kuta-lge commented Mar 23, 2020

I disagree, It shows why it's important to implement this change.

Wouldn't still be important to implement it even if the recipes were identical?

@shr-project
Copy link
Contributor Author

Wouldn't still be important to implement it even if the recipes were identical?

No it wouldn't be important, because bitbake doesn't care from which directory the recipe was used, but when the built output is very different, then it's important to use the new recipe.

@herb-kuta-lge
Copy link
Contributor

even if the recipes were identical

Actually, if a recipe moves to a different subdirectory, it's ROS_CN setting will be different, so the recipes will never be identical.

@herb-kuta-lge
Copy link
Contributor

herb-kuta-lge commented Mar 24, 2020

I don't see how showing how different the recipes

I disagree, It shows why it's important to implement this change.

The details of the differences are unimportant, so perhaps show how different they are by piping the diff output into diffstat? (Use diff -C 1 so that the filename appears in the diffstat output.)

@shr-project shr-project force-pushed the jansa/ros-clean branch 2 times, most recently from 8ba8338 to 225fc83 Compare April 15, 2020 07:58
Copy link
Contributor

@herb-kuta-lge herb-kuta-lge left a comment

Choose a reason for hiding this comment

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

It's a bit confusing, but I think what else is left to resolve are:

  • Replacing the long diff in the commit message with something shorter, eg, diffstat output.
  • Having a single list of the files to be removed and use it when outputting their names and calling git rm.
  • Comments explaining what would cause unexpected output from git status --porcelain and glob.

@shr-project
Copy link
Contributor Author

Comments explaining what would cause unexpected output from git status --porcelain and glob.

Can you please provide these comments?

I don't know what unexpected might have happen especially in combination with --only which isn't implemented yet AFAIK.

@herb-kuta-lge
Copy link
Contributor

I don't know what unexpected might have happen especially in combination with --only which isn't implemented yet AFAIK.

If it's unexpected, shouldn't it be considered an ASSERT that stops execution?

Copy link
Contributor

@herb-kuta-lge herb-kuta-lge left a comment

Choose a reason for hiding this comment

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

Remaining issues:

  • Should the unexpected output that generates the errors on lines 72 and 88 be considered assertion failures and cause the program to exit? Otherwise, isn't it possible that the errors will be missed (because the output from a normal run is so verbose)?
  • Commit message tweak; change the final : to a ..

* clean_ros_recipe_dirs() wasn't called anywhere, update it for what's
  now generated by superflore and call it when regenerating all of the
  recipes.

* the overlay.repo.remove_file(existing, True) call in bitbake/gen_packages.py
  works fine, but only when the new recipe is being generated in the same directory
  as the previous version:
  existing = glob.glob('{}_*.bb'.format(prefix))
  where prefix is:
  prefix = '{0}/meta-ros{1}-{2}/generated-recipes/{3}/{4}'
  which will result in multiple recipes for the same package when the
  component that provides a package is changed like what happened to
  pcl-conversions in dashing and eloquent.

Signed-off-by: Martin Jansa <martin.jansa@lge.com>
@shr-project
Copy link
Contributor Author

The output is verbose, but err() output is quite rare in it and contains even more important messages than this unexpected output, see the errors in last meta-ros regeneration:
http://paste.ubuntu.com/p/b6JbJyTp8d/

@herb-kuta-lge
Copy link
Contributor

OK, then let's leave it as is.

@herb-kuta-lge herb-kuta-lge merged commit 0be23cc into ros-infrastructure:master Apr 22, 2020
@herb-kuta-lge herb-kuta-lge added this to the Version 0.3.2 milestone Apr 22, 2020
@herb-kuta-lge
Copy link
Contributor

@allenh1 < Now that this last open PR was been merged, could you release a v0.3.2 ?

@allenh1
Copy link
Contributor

allenh1 commented Apr 22, 2020

@herb-kuta-lge That sounds like a good idea to me!

@tfoote FYI

@shr-project shr-project deleted the jansa/ros-clean branch April 23, 2020 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants