Implementation of REP 143 #45

dirk-thomas · 2014-11-25T20:24:05Z

No description provided.

tfoote · 2014-11-25T22:43:14Z

There are no tests for index_v3 at the moment.

tfoote · 2014-11-25T22:46:38Z

src/rosdistro/__init__.py

@@ -230,6 +250,7 @@ def get_release_cache(index, dist_name):


 def get_release_builds(index, release_file):
+    print("# rosdistro.get_release_builds() has been deprecated and its functionality is now handled by the 'ros_buildfarm.config' module", file=sys.stderr)


in this and the following warnings can we detect which version we're using? If so it should say that it will return an empty list always in that case.

If the index file has been updated to version 3 it can not list any build files. Therefore I don't think a message these function will return an empty list is necessary. They simply return what is specified in the yaml.

dirk-thomas · 2014-11-25T22:56:02Z

Looks like I forgot to merge some changes from here: https://github.com/dirk-thomas/ros-infrastructure_rosdistro/commit/317a641a47b16ec30cc2afc4ed6d3e6215d946da
That will at least make sure that the new API return the expected information.

gerkey · 2014-11-26T18:20:29Z

scripts/rosdistro_migrate_to_rep_141

@@ -45,7 +45,7 @@ def migrate(index_yaml):
        update_cache(index_yaml, distro_name, cache_url, cache_file, distribution_file)

    data = index_to_yaml(data)
-    data = '\n'.join(_yaml_header_lines('index')) + '\n' + data
+    data = '\n'.join(_yaml_header_lines('index', data['version'])) + '\n' + data


Why are changes needed for the existing 137 -> 141 migrator? Is this fixing an old bug?

The function signature changed and now requires a version argument. The behavior should stay the say i this case.

gerkey · 2014-11-26T18:26:58Z

src/rosdistro/manifest_provider/git.py

@@ -57,6 +57,7 @@ def _get_package_xml(url, tag):
    base = tempfile.mkdtemp('rosdistro')
    try:
        # git 1.7.9 does not support cloning a tag directly, so doing it in two steps
+        assert _git_client_executable is not None, "'git not found"


Is the leading single quote on git intentional?

Absolutely - but it should also have a ' after it 😉

wjwwood · 2014-11-26T18:37:21Z

src/rosdistro/index.py

+                # for backward compatibility only
+                for key in ['release_builds', 'source_builds', 'doc_builds']:
+                    if key not in self.distributions[distro_name]:
+                        self.distributions[distro_name][key] = []


Should this only be done in the case of version 3? What happened before if the file did not have these entries?

Yes, in version 3 that will always be the case.

I version 2 that was never the case since we always had the entries. But if you would have left them out I would expect several scripts to break when trying to use them.

So I think it is cleaner to always ensure they are present.

Sounds good to me.

wjwwood · 2014-11-26T18:41:10Z

src/rosdistro/manifest_provider/git.py

@@ -88,6 +91,7 @@ def _get_package_xml(url, tag):
 def check_remote_tag_exists(url, tag):
    base = tempfile.mkdtemp('rosdistro')
    try:
+        assert _git_client_executable is not None, "'git not found"


Same thing here, looks like a leading '

wjwwood · 2014-11-26T18:54:45Z

+1

dirk-thomas · 2014-12-04T21:54:07Z

I added the test_commits and test_pull_requests flags to the source repositories (8d548f5) as well as a list of tag names to distribution files (39e7af7). Please see this comment (ros-infrastructure/rep#87 (comment)) what the tags will be used for.

dirk-thomas · 2014-12-07T21:00:48Z

+1 (self-vote) on merging this and releasing rosdistro 0.4.0.

tfoote · 2014-12-08T07:50:46Z

I have done a preliminary full run through validating the use of REP 143 on a custom buildfarm. I think it will work. We need to keep track to return and add unit tests, but I think having the draft implementation available from the standard toolchain will be very valuable.

Implementation of REP 143

mkoval · 2014-12-09T07:07:10Z

I have been testing this out locally in a virtualenv by chaining a custom rosdistro with the OSRF rosdistro. So far, I've encountered two problems:

All distribution files need to be the same version (assertion on distribution_file.py:83), which generally seems like a prudent default. However, the only difference between V1 and V2 is the addition of tags, so it seems safe to allow heterogeneous versions in this case. (This may be a non-issue once the OSRF rosdistro adopts REP-143).
All distribution files need to share the same release platforms (assertion on distribution_file.py:84). Instead, it would be ideal if the release platforms listed in the last distribution file entry "win." This would let a custom rosdistro build for a subset of the platforms supported by OSRF.

Otherwise, these changes are great. This should significantly reduce the number of workarounds necessary to run a local buildserver! 👍

dirk-thomas · 2014-12-09T07:27:18Z

@mkoval

Since rosdistro provides API for backward compatibility which merges all distribution files it is safer to require them to have the same version number since user land code might check the version of the distribution file. For heterogeneous versions this would potentially break user land code which relies on conditional logic based on the version.
The release platforms are used at release time of each package by bloom. So adding another distribution file with more platforms will not make all the previously released packages of the other distributions files being released for these new platforms. And allowing to remove platforms from the list doesn't seem valuable. Therefore the merge enforces a consistent set of platforms. Also the distribution files do not determine which targets are actually being built - that is done by the build files in the ros_buildfarm_config repo. They only specify which platforms bloom creates releases for in the gbp repositories.

If you see a way to relax the constraints while maintaining full backward compatibility I am more than happy you consider it. The REP is currently only in "draft" status and can be modified until marked final.

mkoval · 2014-12-09T08:12:54Z

@dirk-thomas

I figured as much. That's fine by me because this is only a transient issue.
I agree that it does not make sense to add additional release platforms, but I do think it would be valuable to remove platforms from the list. There are a few reasons why:
1. Not all packages will build on all release platforms. For example, the necessary dependencies may not exist for Fedora. This is not unique to Fedora: I have been burned by rosdep keys changing names when moving between releases on the same OS.
2. Any downstream rosdistro will break when a new release platform is added to an upstream rosdistro, even if it is not being used. This is, of course, a risk of referencing the upstream rosdistro by URI. However, it would still ideal to minimize the number of cases that require manual intervention.
3. (minor) Releasing for several extra platforms unnecessarily increases the amount of noise in the GBP repositories. This is a pain if you have to apply any distribution-specific patches as part of the release process.
4. This should not break backwards compatibility, since listing multiple distribution files is only supported in V2.

You may already deal with (ii) in the Bloom release process. However, my understanding is that bloom-release iterates over each release platform in the distribution file and performs a separate git-bloom-release rosrelease and git-bloom-release rosdebian step. The corresponding rosdebian step would fail if the correct rosdep keys are not available to generate the Debian package metadata.

In general, I don't think a downstream rosdistro should have to "pay for" any of the release platforms that it does not explicitly support.

wjwwood · 2014-12-09T18:11:36Z

@mkoval bloom does not use the distribution files to determine what actions it takes.

There is a default list of actions in the tracks.yaml file in the master branch of the release repository which determines which generators get called:

https://github.com/ros-gbp/ros_comm-release/blob/master/tracks.yaml#L68-71

For instance we recently added the fedora generator to that list.

Also the packages which get released are determined by looking at the packages in the upstream repository and excluding packages which have been explicitly ignored using a <rosdistro>.ignore file in the master branch of the repository, for example:

https://github.com/ros-gbp/ros_comm-release/blob/master/indigo.ignored

mkoval · 2014-12-09T18:20:26Z

@wjwwood The tracks.yaml file does not list release platforms. It only specifies the ROS distribution and steps in the release process. Where does Bloom get that information, if not from the distribution file?

Edit: To be more specific, the rosdebian step iterates over release platforms to generate Debian meta-data.

wjwwood · 2014-12-09T18:51:59Z

@mkoval The user must explicitly setup a track for each release. bloom does not require the distribution file to operate, only to automate the cloning and pushing of the release repository and for opening pull requests.

You can use bloom to create a release repository for a repository of packages without any input from the distribution file.

wjwwood · 2014-12-09T18:53:37Z

Edit: To be more specific, the rosdebian step iterates over release platforms to generate Debian meta-data.

I see what you mean, yes it gets which ubuntu/debian platforms from the distribution file via rosdep. This is done through rosdep for historical reasons, but this is a different piece of information from deciding which generators (rosdebian/rosrpm) are run.

mkoval · 2014-12-09T20:00:23Z

@wjwwood Yes, you're absolutely right. I conflated generators with release platforms in my original post. You can always remove generators from the tracks.yaml file in your GBP repository. This would address the Fedora example that I used in that post.

However, I am still concerned about the Ubuntu/Debian/Fedora platforms read from the distribution files. E.g. a custom rosdistro only supports raring, then it should not be necessary to also release packages for precise and quantal just because the OSRF rosdistro does so.

wjwwood · 2014-12-11T00:49:25Z

@mkoval I understand your point, and from a idealogical stand point I tend to agree that this information doesn't belong in the distribution file. However, I believe that decoupling the platforms from the repository entries is not sufficient.

All of the release repositories listed in the distribution.yaml file have branches and tags for the platforms listed in that same file, but not necessarily others.

Let's say you wanted to reference our indigo distribution file (whose default platforms are saucy and trusty) and instead build it for utopic. Even if you can change or override the platforms we set for our distribution file, the release repositories referenced in that distribution file do not contain what you need to build those packages for utopic. So you would need to re-bloom those packages to update the release repositories.

In some unintended way the platforms being listed in the distribution file describe the minimal contents of the release repositories that are referenced within.

mkoval · 2014-12-16T05:56:39Z

It is fine for the distribution file to specify the minimum set of branches and tags that will be present in the GBP repositories. This means that each distribution file listed in the index.yaml must contain a subset of the distributions specified in the previous distribution files. E.g. if OSRF releases for saucy and trusty, then the user should be able to only specify trusty in his or her own distribution file.

As you pointed out, It would not be okay for the user to also specify precise. This could throw an error, just like rosdistro currently does if the sets do not exactly match.

(Sorry for the delay. I just realized that this was sitting as a draft for a few days.)

dirk-thomas · 2014-12-16T19:34:42Z

I implemented the relaxed condition that release_platforms can be a subset of the previous distribution file: #46

dirk-thomas added 3 commits November 25, 2014 10:59

rename test index.yaml to include version

6838dae

add index format 3 features

6dc6909

update writer and verify, add migration script

9abb479

dirk-thomas force-pushed the rep143 branch from e2aebf1 to 9abb479 Compare November 25, 2014 21:30

tfoote reviewed Nov 25, 2014
View reviewed changes

update tests to use index_v3.yaml

358739a

dirk-thomas force-pushed the rep143 branch from c7d2a80 to 358739a Compare November 25, 2014 22:59

dirk-thomas added 6 commits November 25, 2014 16:41

update cache to handle multiple distro files correctly

ac1eb44

add missing import __future__

36a8be3

fix modifying dict while iterating over it

e6158e2

fix cache with Python 3

e2d630d

fix Python 2 loading when cache is generated with Python 3

1aeb88b

add assertion before using git client executable

5d94177

dirk-thomas force-pushed the rep143 branch from 9245b07 to 5d94177 Compare November 26, 2014 03:18

fix changes from ac1eb44

fd3281b

gerkey reviewed Nov 26, 2014
View reviewed changes

fix deprecation message

4f9dfd3

gerkey reviewed Nov 26, 2014
View reviewed changes

fix assertion message

8a00759

wjwwood reviewed Nov 26, 2014
View reviewed changes

improve comment

71fdc27

wjwwood reviewed Nov 26, 2014
View reviewed changes

remove unnecessary conversion

a2a6272

add support to test_commits / test_pull_requests flags

8d548f5

wjwwood mentioned this pull request Dec 1, 2014

Consider multiple distribution files with REP-143 ros-infrastructure/bloom#330

Merged

add tags to distribution files

39e7af7

tfoote added 2 commits December 5, 2014 16:45

fixing copy and paste error

3ae09f9

fixing copy and paste error

937a1a9

dirk-thomas added a commit that referenced this pull request Dec 8, 2014

Merge pull request #45 from ros-infrastructure/rep143

3381c74

Implementation of REP 143

dirk-thomas merged commit 3381c74 into master Dec 8, 2014

dirk-thomas deleted the rep143 branch December 8, 2014 18:11

dirk-thomas restored the rep143 branch December 8, 2014 20:01

dirk-thomas deleted the rep143 branch December 8, 2014 22:19

dirk-thomas mentioned this pull request Dec 16, 2014

relax release platforms check when merging distribution files #46

Merged

dirk-thomas mentioned this pull request Mar 30, 2015

mark REP 143 final ros-infrastructure/rep#98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of REP 143 #45

Implementation of REP 143 #45

dirk-thomas commented Nov 25, 2014

tfoote commented Nov 25, 2014

tfoote Nov 25, 2014

dirk-thomas Nov 25, 2014

dirk-thomas commented Nov 25, 2014

gerkey Nov 26, 2014

dirk-thomas Nov 26, 2014

gerkey Nov 26, 2014

dirk-thomas Nov 26, 2014

wjwwood Nov 26, 2014

dirk-thomas Nov 26, 2014

wjwwood Nov 26, 2014

wjwwood Nov 26, 2014

wjwwood commented Nov 26, 2014

dirk-thomas commented Dec 4, 2014

dirk-thomas commented Dec 7, 2014

tfoote commented Dec 8, 2014

mkoval commented Dec 9, 2014

dirk-thomas commented Dec 9, 2014

mkoval commented Dec 9, 2014

wjwwood commented Dec 9, 2014

mkoval commented Dec 9, 2014

wjwwood commented Dec 9, 2014

wjwwood commented Dec 9, 2014

mkoval commented Dec 9, 2014

wjwwood commented Dec 11, 2014

mkoval commented Dec 16, 2014

dirk-thomas commented Dec 16, 2014

		@@ -230,6 +250,7 @@ def get_release_cache(index, dist_name):


		def get_release_builds(index, release_file):
		print("# rosdistro.get_release_builds() has been deprecated and its functionality is now handled by the 'ros_buildfarm.config' module", file=sys.stderr)

Implementation of REP 143 #45

Implementation of REP 143 #45

Conversation

dirk-thomas commented Nov 25, 2014

tfoote commented Nov 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 25, 2014

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

wjwwood commented Nov 26, 2014

dirk-thomas commented Dec 4, 2014

dirk-thomas commented Dec 7, 2014

tfoote commented Dec 8, 2014

mkoval commented Dec 9, 2014

dirk-thomas commented Dec 9, 2014

mkoval commented Dec 9, 2014

wjwwood commented Dec 9, 2014

mkoval commented Dec 9, 2014

wjwwood commented Dec 9, 2014

wjwwood commented Dec 9, 2014

mkoval commented Dec 9, 2014

wjwwood commented Dec 11, 2014

mkoval commented Dec 16, 2014

dirk-thomas commented Dec 16, 2014