Refactor generator #539

lennonwoo · 2019-06-17T00:30:35Z

Motivation

As far as we can see, the current code base of generators exist some duplicate code, which will be hard for developers to maintain. And since ROS2 will be release to Mac, Windows oneday in the future. It will be harmful when we want Bloom to support another release platform if we still use the duplicate code at that time.

The refactor

I go through the code of debian and rpm's generator and find out there are some functions we could reuse. Including:

The place/process tempalte files logic
The generate_substitutions_from_package function
prepare_arguments, pre_modify, get_branching_arguments and etc as descripted in BloomGenerator
The set_original_config load_original_config functions
The set_releaser_history get_releaser_history functions

But there are also something different between debian and rpm's generator. Like:

The format of debian and rpm's changelog, date are difference, as you can see in debian, rpm
Debian need debhelper_version substitution, but Rpm need NoArch substitution.
The details of generate package logic is different between rpm and debian's generator, as you can see in generate_debian, generate_rpm
Currently, Rpm's generator didn't support ament_python and ament_cmake since PR Add Ament package support to RPM generation #534 haven't merged

refactor details

For common part as discussed above, I handle as follow:

I move related function as described in point 1, 2 to common.py
I create a new base class named PackageManagerGenerator to store functions described in point 3, 4, 5

For diverse part as discussed above, I handle as follow:

For point 1 and 2, I use get_subs_hook to handle package manager specific substitutions. There I mark this function as staticmethod since we also use this hook for local packages generation when someone use bloom_generate command.
I use virtual generate_package for successor to overwrite to handle point 3
For point 4, I expect the PR as discussed above to be merged first, so I there simply use debian's logic in common.py

The tests I used

First I use some existing ROS package to test if the refactor will change the Bloom output files' content, the tests show that the refactor won't change the contents.
I also add some unit tests including:
1. test_set_default_distros to test we can set default distros as before if we only input os_name and rosdistro as args
2. test_place_template_files to test the correctness of logic if package already have some unique files under its package manager folder
3. test_bad_dependency to test Bloom still complain the unresolvable dependencies as before
4. test_get_substitute to test the successor of PackageManagerGenerator could do work as expected.

These tests may be not strong enough to let this PR merged and if you think something would matters and should have a test, ping me.

Some plan and questions

As we can see in the PR, there still exist some duplicate in ROS related generator class, generate_cmd related method, these could be refacotred in the future.

The generate_tag_name in debian and generate_tag_name in rpm seems weird, could we use the same format?

The update_rosdep step will takes long times when we nosetest Bloom or generate packages, we may need optimize it.

That's all, thank you!

seanyen · 2019-06-20T00:04:18Z

It would be great if Windows and MacOS can be added to the TravisCI configuration so in future we can be easier to see if any regression on other platforms.

lennonwoo · 2019-06-20T04:59:16Z

@seanyen That will be nice if we support Windows and MacOS test in TravisCI. But there are some problems we need to solve to support it:

First our code base exist some conflict(like mkdir, os.symlink, etc.) between Linux and other platforms.
Then the Bloom dependencies like catkin_pkg itself don't have CI in Windows or MacOS.
In another side, the TravisCI doesn't support Python language officially, see its doc, and their discussion

nuclearsandwich

It's looking good so far. I've left a handful of comments that merit follow-up but nothing show-stopping.

nuclearsandwich · 2019-06-21T16:34:04Z

bloom/generators/rosrelease.py

@@ -62,22 +62,4 @@ def post_patch(self, destination):
            name, version, packages = get_package_data(destination)
        # Execute git tag
        execute_command('git tag -f ' + destination + '/' + version +
-                        '-' + str(self.release_inc))
-
-    def detect_branches(self):


@lennonwoo for future similar changes it would be worth it to mention in your commit message that this change doesn't cause breakages. It's not clear from the commit alone that detect_branches is implemented on ReleaseGenerator and duplicated here. It looks like you're deleting a method that is still in use.

Thank you, I add the extra commit message about it in the corresponding commit.

nuclearsandwich · 2019-07-02T19:59:31Z

bloom/generators/rosdebian.py

        ])
        return args

    def get_release_tag(self, data):
        return 'release/{0}/{1}/{2}-{3}'\
-            .format(self.rosdistro, data['Name'], data['Version'], self.debian_inc)
+            .format(self.rosdistro, data['Name'], data['Version'], self.inc)


Let's use package_revision or revision rather than just inc. Likewise for all the modified template files.

revision is definitely a better name. But I am not sure where should I rebase my commit. And also notice that -inc is the template action we use, maybe this should an extra commit?

nuclearsandwich · 2019-07-02T21:23:50Z

bloom/generators/common.py

+try:
+    import rosdistro
+except ImportError as err:
+    debug(traceback.format_exc())


Do you need to pass the exception err here? Otherwise why does err need to be named?

nuclearsandwich · 2019-07-02T21:33:34Z

bloom/generators/common.py

@@ -363,3 +397,89 @@ def post_patch(self, branch_name):
        :returns: return code, return 0 or None for OK, anythign else on error
        """
        return 0
+
+
+class PackageSystemGenerator(BloomGenerator):


I'm not sure yet about this class name, but I think that there's enough here it's worth putting the class in a file of its own. Second opinion @cottsay?

nuclearsandwich · 2019-07-02T21:35:38Z

bloom/generators/debian/generator.py

-            error("No platforms defined for os '{0}' in release file for the '{1}' distro."
-                    .format(self.os_name, self.rosdistro), exit=True)
-        self.distros = distribution_file.release_platforms[self.os_name]
+        return PackageSystemGenerator.prepare_arguments(self, parser)


Shouldn't this be return self.prepare_arguments(parser) to allow for overloading?

There I think we need to call super's prepare_argument to reuse the same prepare login in parent generator. But I follow the style of the current codebase: https://github.com/ros-infrastructure/bloom/blob/master/bloom/generators/release.py#L75

nuclearsandwich · 2019-07-02T21:38:02Z

bloom/generators/debian/generator.py

-                .format(self.package_system))
-        if type(packages) is dict:
-            return list(packages.values())[0]
+        ret = PackageSystemGenerator.handle_arguments(self, args)


Why not use super() here?

nuclearsandwich · 2019-07-02T22:47:44Z

bloom/generators/common.py

@@ -841,7 +841,7 @@ def _check_all_keys_are_valid(self, peer_packages, rosdistro):
                    all_keys_valid = False
        return all_keys_valid

-    def _pre_modify(self, key_unvalid_error_msg):
+    def check_all_keys_are_valid(self, key_unvalid_error_msg):


I think key_unvalid is probably a typo of key_invalid that we can fix while we're here.

Fixed in the newest commit

Notice that the `get_package_data` method will return a list. so it's clear that the logic `detect_branches` method in ReleaseGenerator and RosReleaseGenerator is same. In this commit, I reuse this function and put it into ReleaseGenerator's `handle_arguments` method so that we can safely delete the duplicate part in RosReleaseGenerator.

We need to put the `missing_dep_resolver` in class as staticmethod in case successor of Generator want to reuse this function. And I also add os_name, os_versino, ros_distro if oneday the resolver need more dependency information.

Firstly put the common place/process template files to common.py. Then, since every pacakge_system has its prefer way of formatting description and depends, so pass them as function. Next, rewrite get_sub_hook in DebianGenerator for debian specific substitutions attribute, and consider generate_cmd will use the hook, I make the hook as staticmethod method. At last, fix up the api change influence.

The major different part of RPM and Debian's generator as follow: 1. rpm don't have native option in local generate_cmd related command 2. rpm and debian's Date, changelogs, and License format are different 3. rpm need NoArch substitution 4. rpm has its description and depends specification And I also update the utils function like __process_template_folder, __place_template_folder, and place_tempalte_folder, the newest code in debian should also fit rpm generator's logic

Also fix typo here(unvalid => invalid)

This commit add the missed rosdistro when call super's get_subs_hook in rosdebian or rosrpm. The release_history should be passed as **kwargs format since it have default value already. And I also combine two line in get_release_tag method since it's not exceed 120 lines.

Neither Fedora or RHEL 7+ use this value. If another distribution were to be supported for RPM generation in Bloom that can take advantage of this value, we should find a better way to add it instead of hard- coding it into the template. Signed-off-by: Scott K Logan <[email protected]>

Supported in RPM 4.11 and newer: https://rpm.org/user_doc/autosetup.html Signed-off-by: Scott K Logan <[email protected]>

...rather than assuming that the make executable is 'make'. Signed-off-by: Scott K Logan <[email protected]>

Also move terminating '..' to the end to make it easier to patch cmake arguments and less likely that the make arguments accidentally get added to the end of the cmake arguments. Signed-off-by: Scott K Logan <[email protected]>

There are two classes of issues here: 1. ROS packages often provide libraries which are also provided by the operating system. If an operating system declares a dependency on that library, we don't want the package manager to install the ROS package instead of the system package. 2. Many ROS packages don't install libraries in a way that the 'provides' portion of dependency generation can detect, so when another ROS package takes a dependency on that library, the automatic dependency can't be met and the downstream package cannot be installed. More info on RPM dependency generation: https://rpm.org/user_doc/dependency_generators.html Signed-off-by: Scott K Logan <[email protected]>

The BRP Python bytecompiler will always use the sytem's default Python interpreter, which may not be the interpreter we're targeting. Safest option is to disable the automagic byte compilation altogether. Note that this doesn't mean that python files won't ever be compiled, it just means that the catch-all policy at the end of the process won't attempt to compile anything which hasn't already been compiled. Signed-off-by: Scott K Logan <[email protected]>

This change shouldn't modify the behavior in Fedora, where all current releases define 'cmake3' to be the same as 'cmake'. In RHEL 7, where cmake 2 is the default, we need to use the 'cmake3' macro to use the supplamental 'cmake3' executable instead of the system default. All current ROS releases except Indigo have a *minimum* cmake requirement of 3. Signed-off-by: Scott K Logan <[email protected]>

…e#540) The SCM plugin for mock will create an archive of the sources if either this marker file is present in the root of the project or the `write_tar` option is specified in mock's configuration. The way bloom works, we always want to archive the sources, but this isn't the default configuration of the SCM plugin. Writing this marker file means one less configuration is necessary when consuming the release repo to build the RPMs with mock. Signed-off-by: Scott K Logan <[email protected]>

nuclearsandwich reviewed Jul 2, 2019

View reviewed changes

lennonwoo closed this Jul 3, 2019

lennonwoo deleted the refactor_generator branch July 3, 2019 11:13

lennonwoo restored the refactor_generator branch July 3, 2019 11:13

lennonwoo added 15 commits July 3, 2019 19:17

reuse post_rebase in release generator

05363ff

change resolver into class's staticmethod

41037cb

We need to put the `missing_dep_resolver` in class as staticmethod in case successor of Generator want to reuse this function. And I also add os_name, os_versino, ros_distro if oneday the resolver need more dependency information.

use common Inc and package system

0495da6

put class relate method to where it belongs

fba38ed

PackageSystemGenerator for collecting common process arguemnt function

0c9693d

common function when generate packages files

c10869f

use pacakge manager as base generator's name

1f5a705

add set_default_distros test

452fece

test place template files logic added

263275f

check_all_key_are_valid function test added

d2ef245

Also fix typo here(unvalid => invalid)

test common part of substitutions

d1aa11c

cottsay reopened this Jul 4, 2019

cottsay force-pushed the refactor_generator branch from 2449c99 to 8c23028 Compare July 4, 2019 00:24

nuclearsandwich self-requested a review July 15, 2019 22:16

cottsay added 6 commits August 10, 2019 15:30

Use 'autosetup' macro in RPM spec files

5db12cc

Supported in RPM 4.11 and newer: https://rpm.org/user_doc/autosetup.html Signed-off-by: Scott K Logan <[email protected]>

Use 'make_*' macros in RPM spec files

ff203da

...rather than assuming that the make executable is 'make'. Signed-off-by: Scott K Logan <[email protected]>

Use more standard 4-space indentation in RPM spec files

fd52e5c

Also move terminating '..' to the end to make it easier to patch cmake arguments and less likely that the make arguments accidentally get added to the end of the cmake arguments. Signed-off-by: Scott K Logan <[email protected]>

cottsay and others added 3 commits August 10, 2019 15:30

Merge remote-tracking branch 'origin/master' into HEAD

970380e

lennonwoo force-pushed the refactor_generator branch from 0321b00 to 970380e Compare August 10, 2019 07:42

lennonwoo mentioned this pull request Aug 10, 2019

Vcpkg support for Bloom #541

Open

nuclearsandwich mentioned this pull request Oct 11, 2019

Add ROS_PYTHON_VERSION to dependency conditionals. #551

Merged

cottsay mentioned this pull request Mar 10, 2020

Move shared generator code to the base class or a core helper #578

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor generator #539

Refactor generator #539

lennonwoo commented Jun 17, 2019 •

edited

Loading

seanyen commented Jun 20, 2019

lennonwoo commented Jun 20, 2019

nuclearsandwich left a comment

nuclearsandwich Jun 21, 2019

lennonwoo Jul 4, 2019

nuclearsandwich Jul 2, 2019

lennonwoo Jul 3, 2019

nuclearsandwich Jul 2, 2019

nuclearsandwich Jul 2, 2019

nuclearsandwich Jul 2, 2019

lennonwoo Jul 3, 2019

nuclearsandwich Jul 2, 2019

nuclearsandwich Jul 2, 2019

lennonwoo Jul 4, 2019

Refactor generator #539

Are you sure you want to change the base?

Refactor generator #539

Conversation

lennonwoo commented Jun 17, 2019 • edited Loading

Motivation

The refactor

refactor details

The tests I used

Some plan and questions

seanyen commented Jun 20, 2019

lennonwoo commented Jun 20, 2019

nuclearsandwich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lennonwoo commented Jun 17, 2019 •

edited

Loading