Orig tarball #374
Orig tarball #374
Conversation
tfoote
force-pushed
the
orig_tarball
branch
2 times, most recently
from
50863cd to
b2cc094
Compare
|
I don't think this should replace the other PRs. E.g. #375 makes a lot of sense even when this gets applied. Otherwise if Yakkety is the first platform to build the tarball contains current time stamps and can't be recreated without a diff. Anyway imo this needs extensive testing on the test farm including, first time releases, new patch releases, new debinc releases, etc. (I will use the "more-information-needed" label for this.) |
|
This is a high priority, deploying this should allow us to turn off the EOL'd distros which we've wanted to do for a long time. The most recent thread is here And had planned to do it for even longer. After working on manually packaging debian increments I believe that this is the better way to package everything. From the debian handbook section the orig.tar.gz is not supposed to be modified by the packaging. @jspricke @j-rivero Can you confirm that this is a correct approach for building the packages? For reference we've discovered that xenial, yakkety, and zesty produce different tarballs since they use different options to tar which means they conflict on trying to upload the orig.tar.gz. #373 and #375 workaround a change in standard and a bug respectively. I agree #375 is less than desireable behavior. we could leave it in. We'll just have to remove that code path once we stop supporting Yakkety. And it's a very fragile fix if there's ever a change to those lines (such as debian makes a patch release to fix the bug, though quite unlikely), the sed line will fail. And with this fix we will never run into this problem since we won't regenerate the orig.tar.gz files. Completely reproducable builds are valuable, but I'd argue that adding the complexity to our system is unnecessary since it's already fixed upstream and should remain so for future versions. |
|
Regarding #375 it is clearly not a great solution. I am happy if anyone suggests a "cleaner" workaround. We could e.g. create our own version of the Debian package and put it in our repo. But since that involves more effort I went for the workaround I am able to implement within minutes. Anyway I disagree with your conclusion of this being especially fragile and increases complexity unnecessarily.
|
|
Hi Tully,
* Tully Foote <[email protected]> [2017-02-16 23:15]:
After working on manually packaging debian increments I believe that this is the better way to package everything. From the [debian handbook section](https://debian-handbook.info/browse/stable/sect.source-package-structure.html) the orig.tar.gz is not supposed to be modified by the packaging.
For Debian the orig.tar.gz should be the one the upstream project
provides¹, so it shouldn't make a difference if you take the one from
the repo or from upstream². As you are not Debian, I think it's up to
you to decide how to handle this. I agree with Dirk, that gbp is buggy,
and it should be fixed on their side (or worked around as in #375, while
you rely on a broken version). Hopefully this doesn't happen again so
often (otherwise your idea is a viable way to work around these).
Cheers Jochen
¹: Except when there is a good reason to repack it, like non-free
content.
²: Except when upstream changes released versions, where I think this is
a nice check for.
|
|
This is currently blocking the Lunar packages for Debian targets. Note that those are actually creating different archives since the newer |
|
@tfoote @j-rivero any input on how to handle the difference between debian and ubuntu for the orig tarballs ? (c.f. #380 (comment)) EDIT: this comment is a duplicate of previous comment from @dirk-thomas |
|
@mikaelarguedas @dirk-thomas the |
|
@tfoote Thanks for clarifying. |
This will avoid conflicts with different tar compression methods as well as hopefully fix the debinc issue.
|
I've rebased on the latest. It now builds on Stretch, Xenial, Yakkety, and Zesty for Lunary. I ran all 4 of these debian increments in parallel: And they all passed. This job passes: http://build.ros.org:8080/view/Lsrc_uX/job/Lsrc_uX__angles__ubuntu_xenial__source/6/ after the old style Yakkety job changed the tarball underneath this job running: http://build.ros.org:8080/job/Lrel_import-package/164/console It now has built both our use cases, a new releases and debian increment releases. There is currently one known issue which is that if two are triggered simultaneously on a new release they might both generate the tarball. And if they generate different ones(aka different behaviors between versions of dpkg like #375 ) In which case it will hit our existing retry logic and whichever job uploaded 2nd will need to retry. It will be better than the current situation where we require that all arches run before any individual one can succeed again. Note that this also fixes the Stretch builds RE: ros-infrastructure/ros_buildfarm_config#75 |
ros_buildfarm/sourcedeb_job.py
Outdated
| url = URL_TEMPLATE % (repo, prefix, debian_package_name, filename) | ||
|
|
||
| try: | ||
| output_file = os.path.join(sources_dir, '..', filename) |
There was a problem hiding this comment.
Please move this line before the try / except block.
ros_buildfarm/sourcedeb_job.py
Outdated
| urlretrieve(url, output_file) | ||
| print("Found matching original tarball, " | ||
| "downloading %s to %s" % (url, output_file)) | ||
| continue |
There was a problem hiding this comment.
continue seems to be unnecessary here. First it does loop anyway. Second should it actually try the other repo urls or break instead?
ros_buildfarm/sourcedeb_job.py
Outdated
|
|
||
| for repo in debian_repository_urls: | ||
| URL_TEMPLATE = '%s/pool/main/%s/%s/%s' | ||
| prefix = debian_package_name[0] |
There was a problem hiding this comment.
Please move these two line before the loop.
ros_buildfarm/sourcedeb_job.py
Outdated
| output_file = os.path.join(sources_dir, '..', filename) | ||
| urlretrieve(url, output_file) | ||
| print("Found matching original tarball, " | ||
| "downloading %s to %s" % (url, output_file)) |
There was a problem hiding this comment.
Please move this line into an else block.
Please put single quotes around the placeholders.
At this point the download should already be complete Therefore the message should be something like this:
Downloaded original tarball %s from %s
There was a problem hiding this comment.
The first sentence still applies:
Please move this line into an else block.
ros_buildfarm/sourcedeb_job.py
Outdated
| "downloading %s to %s" % (url, output_file)) | ||
| continue | ||
| except: | ||
| print("No tarball found at %s, that's ok it will be rebuilt" % url) |
There was a problem hiding this comment.
Please put single quotes around the placeholders.
The part of the message ("it will be rebuilt") might be misleading since it might still be downloaded from another repo. Therefore I would suggest to leave that part out.
|
@dirk-thomas Good points, comments addressed. The continue was supposed to be a break. |
|
@j-rivero has added a +1 to the above comment #374 (comment) |
…ildfarm#374 is merged" This reverts commit 7103f29.
|
We will merge this before the automatic reconfigure of the source jobs tonight. |
|
Manually squashed into 06e7b6c |
|
Just for the record: this patch seems to be the reason for this regression: ros/rosdistro#14091 (comment) |
This will auto commit changes to the upstream tarball as diffs for the debian increment. Addresses issue in #374
* add support for changing tarballs. This will auto commit changes to the upstream tarball as diffs for the debian increment. Addresses issue in #374 * fix spelling * Change to append * making sure there's a new line * wrap
First commit worked here: http://build.ros.org/view/Lsrc_uZ/job/Lsrc_uZ__catkin__ubuntu_zesty__source/5/console
2nd commit adds build file based parameterization and passes here: http://build.ros.org/view/Lsrc_uZ/job/Lsrc_uZ__catkin__ubuntu_zesty__source/7/console and http://build.ros.org/view/Lsrc_uX/job/Lsrc_uX__catkin__ubuntu_xenial__source/3/console
I manually modified the config to use this branch.
I believe that this will fix the issue with debian increments as well.